timers: Fail early when callback is not a function by addaleax · Pull Request #4362 · nodejs/node (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation22 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

setTimeout() and setInterval() currently throw errors when receiving non-function objects as their first argument, but only do so when trying to execute the callback, i.e. after the waited time has passed. This may complicate debugging when a lot of calls to setTimeout()/setInterval() from different places are involved, so failing as early as possible seems like a good idea.

Strangely, setTimeout() currently silently ignores an falsy first argument, while setInterval() does not. This patch leaves this behaviour unchanged since existing applications may rely on it.

r-52 added the timers

Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

label

Dec 20, 2015

Fishrock123 added the semver-major

PRs that contain breaking changes and should be released in the next major version.

label

Dec 21, 2015

I think this is semver-major anyways, in which case you might as well just make both the same (throw on not-function).

cjihrig

@@ -177,6 +177,11 @@ exports.enroll = function(item, msecs) {


exports.setTimeout = function(callback, after) {
// a falsy callback has been okay historically
if (callback && typeof callback !== 'function') {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need the callback &&.

Okay, I rebased against master & applied your proposed changes, i.e. made setTimout(false, …) throw, too.

cc @nodejs/ctc since this is a major.

Want to go ahead and do the same w/ setImmediate()?

Also, should come w/ a docs change explaining this will happen.

Okay, I just updated with the changes to setImmediate() (sorry, overlooked that one).

I’d be happy to write some docs on it, but I’m not exactly sure what you are referring to: A remark in timers.markdown on the modified behaviour? It barely changed – at least for setInterval() and setImmediate() – and I’m not sure it would be worth making the docs there less concise; in fact, I’d rather have expected set*() to throw errors immediately than after the delay. If you’re talking about some sort of changelog, please point me to it (I mean, there’s obviously CHANGELOG.md, but that does not seem to be the right place for explanations).

@addaleax What @Fishrock123 said. For example, at the end of setImmediate(callback[, arg][, ...]) something like:

If callback is not a function setImmediate() will throw immediately.

Nothing big. It's mainly to document the behavior for future changes.

@addaleax mind to update the commit description?

Also just as a note on Semver-Majors, that means this will land in v6. :)

setTimeout(), setInterval() and setIntermediate currently throw errors when receiving non-function objects as their first argument, but only do so when trying to execute the callback, i.e. after the waited time has passed. This may complicate debugging when a lot of calls to setTimeout()/etc. are involved, so failing as early as possible seems like a good idea.

setTimeout() historically ignored an falsy first argument, while the other functions do not and throw instead. This patch changes this behaviour to make all three match and adds remarks in the corresponding documentation.

Added a note to the commit message saying that it updates the docs; I guess that’s what you meant? (Thank you all for your patience with me getting it right… :) )

rvagg pushed a commit that referenced this pull request

Jan 4, 2016

PR-URL: #4362 Reviewed-By: Jeremiah Senkpiel fishrock123@rocketmail.com Reviewed-By: Colin Ihrig cjihrig@gmail.com Reviewed-By: Brian White mscdex@mscdex.net Reviewed-By: Trevor Norris trev.norris@gmail.com Reviewed-By: Rod Vagg rod@vagg.org

Landed @ ac153bd

Thanks for this change @addaleax, having consistency is great! I believe this is your first commit to core, if so, welcome on board and we hope you find other ways to contribute!

@nodejs/documentation & @jasnell: Note the additions in timers.markdown here about throwing when the callback isn't a function. It's nice to have this explicitly called out but grep tells me that we're not doing anything consistently on this even though we've had a lot of changes this year which tighten up the callback==function checks. It'd be great if we could have explicit details on what typechecking is done on arguments and have a consistent way of showing that in the docs.

addaleax deleted the timer-throw-with-invalid-fn branch

January 4, 2016 04:14

@rvagg ... agreed. Once I'm done with the refactoring pass through the remaining half of the documentation I'll be going back through and fixing up additional details. One bit that we're definitely missing throughout the documentation is a clear understanding of error handling in general.

mfun mentioned this pull request

Jan 4, 2016

rvagg mentioned this pull request

Jan 8, 2016

scovetta pushed a commit to scovetta/node that referenced this pull request

Apr 2, 2016

PR-URL: nodejs#4362 Reviewed-By: Jeremiah Senkpiel fishrock123@rocketmail.com Reviewed-By: Colin Ihrig cjihrig@gmail.com Reviewed-By: Brian White mscdex@mscdex.net Reviewed-By: Trevor Norris trev.norris@gmail.com Reviewed-By: Rod Vagg rod@vagg.org

jasnell added a commit that referenced this pull request

Apr 26, 2016