Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

timers: throw on all invalid timer delays #8823

Closed
wants to merge 3 commits into from
Closed

timers: throw on all invalid timer delays #8823

wants to merge 3 commits into from

Conversation

cjihrig
Copy link

@cjihrig cjihrig commented Dec 4, 2014

This commit causes all timer functions to throw if the provided delay is anything other than a non-negative, finite number. No type coercion is performed, as it can lead to subtle bugs.

This is a rewrite of #8739 to target the 0.12 branch. The original PR was opened to address #8618. On #8739 and IRC, it was decided that the timer API would be made as strict as possible to avoid subtle bugs. This required updating a number of existing tests that did not pass a second argument to setTimeout() and setInterval().

R= @misterdjules or @tjfontaine

This commit causes all timer functions to throw if the provided
delay is anything other than a non-negative, finite number. No
type coercion is performed, as it can lead to subtle bugs.
Conflicts:
	test/simple/test-asynclistener-error.js
	test/simple/test-asynclistener.js
@bnoordhuis
Copy link
Member

This breaks browser compatibility rather forcefully and I don't think that's a good thing; people do share code between the client and the server. setTimeout(fn) has a well-defined meaning; I think that should keep working.

@misterdjules
Copy link

@cjihrig My understanding was that we would make the internal timers' API (_unrefActive, etc.) and the Node.js specific APIs (net.Socket.prototype.setTimeout, etc.) stricter, but that we would not change external APIs that are shared with browsers.

I'm sorry if I didn't communicate that properly during our conversation.

@cjihrig
Copy link
Author

cjihrig commented Dec 9, 2014

@misterdjules my understanding of how to move forward with this PR was based off of this conversation. To be completely honest, I didn't take into consideration that browser code could be run in Node.

In that case, this could be reduced to validating timers.enroll() and Socket.prototype.setTimeout(). However, that would lead to inconsistent APIs. Life was much simpler with my original commit.

@cjihrig cjihrig closed this Dec 9, 2014
@misterdjules
Copy link

@cjihrig OK, it's possible that @tjfontaine meant that we want to make setTimeout and setInterval throw on invalid input and that I'm missing something, let's wait for his feedback.

I don't think that only validating for timers.enroll() and Socket.prototype.setTimeout() would make APIs inconsistent, as I consider them to be different APIs than setTimeout.

@cjihrig cjihrig deleted the timers branch December 16, 2014 18:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants