timers: Fail early when callback is not a function #4362

addaleax · 2015-12-20T11:39:08Z

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.

Fishrock123 · 2015-12-21T15:38:09Z

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

cjihrig · 2015-12-21T15:41:42Z

lib/timers.js

@@ -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') {


You don't need the callback &&.

addaleax · 2015-12-21T22:27:59Z

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

Fishrock123 · 2015-12-21T22:38:04Z

LGTM, CI: https://ci.nodejs.org/job/node-test-pull-request/1043/

cjihrig · 2015-12-22T02:32:09Z

LGTM

Fishrock123 · 2015-12-22T14:49:29Z

cc @nodejs/ctc since this is a major.

mscdex · 2015-12-22T16:59:01Z

LGTM

trevnorris · 2015-12-22T17:38:16Z

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

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

addaleax · 2015-12-22T23:44:04Z

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).

rvagg · 2015-12-23T11:50:56Z

lgtm

Fishrock123 · 2015-12-23T18:56:39Z

@addaleax I think Trevor just means to specify in https://github.com/nodejs/node/blob/master/doc/api/timers.markdown#setimmediatecallback-arg- that the callback's must be functions.

trevnorris · 2015-12-23T19:52:44Z

@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 · 2015-12-25T18:05:08Z

@trevnorris @Fishrock123 Updated again, and thanks for the pointers!

Fishrock123 · 2015-12-25T18:43:17Z

@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.

addaleax · 2015-12-25T18:52:55Z

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… :) )

trevnorris · 2015-12-27T03:28:08Z

Thanks. LGTM

cjihrig · 2015-12-30T21:48:10Z

Still LGTM

`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. PR-URL: #4362 Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Rod Vagg <[email protected]>

rvagg · 2016-01-04T03:59:43Z

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 · 2016-01-04T04:14:52Z

Great, and thanks again!

jasnell · 2016-01-04T16:17:26Z

@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.

`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. PR-URL: nodejs#4362 Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Rod Vagg <[email protected]>

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * OS X * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 [#6402](#6402). * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

addaleax force-pushed the timer-throw-with-invalid-fn branch from 4022f3a to f93966d Compare December 20, 2015 12:12

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

cjihrig reviewed Dec 21, 2015
View reviewed changes

addaleax force-pushed the timer-throw-with-invalid-fn branch from f93966d to e35cf28 Compare December 21, 2015 22:22

addaleax force-pushed the timer-throw-with-invalid-fn branch 2 times, most recently from f33d250 to 1964e52 Compare December 22, 2015 23:43

addaleax force-pushed the timer-throw-with-invalid-fn branch from 1964e52 to 7e8b191 Compare December 25, 2015 18:04

addaleax force-pushed the timer-throw-with-invalid-fn branch from 7e8b191 to 8b52afd Compare December 25, 2015 18:50

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:01

rvagg closed this Jan 4, 2016

addaleax deleted the timer-throw-with-invalid-fn branch January 4, 2016 04:14

mfun mentioned this pull request Jan 4, 2016

Overview over all comments for admins tradity/tradity-client#21

Closed

rvagg mentioned this pull request Jan 8, 2016

Docs WG Ratification nodejs/docs#50

Closed

addaleax mentioned this pull request Jan 24, 2016

Async version is not async hiddentao/fast-levenshtein#11

Closed

jasnell mentioned this pull request Mar 17, 2016

Planning for v6 #5766

Closed

jasnell mentioned this pull request Apr 19, 2016

What is new in v6? #6264

Closed

addaleax mentioned this pull request Apr 20, 2016

onImmediate is not a function #6298

Closed

jasnell mentioned this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release #6383

Closed

ChALkeR mentioned this pull request May 2, 2016

API stability level «Locked» is inaccurate #6528

Closed

ChALkeR mentioned this pull request Feb 8, 2017

Changing Definition of "Locked" for Stability Index in API Docs #11200

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

timers: Fail early when callback is not a function #4362

timers: Fail early when callback is not a function #4362

addaleax commented Dec 20, 2015

Fishrock123 commented Dec 21, 2015

cjihrig Dec 21, 2015

addaleax commented Dec 21, 2015

Fishrock123 commented Dec 21, 2015

cjihrig commented Dec 22, 2015

Fishrock123 commented Dec 22, 2015

mscdex commented Dec 22, 2015

trevnorris commented Dec 22, 2015

addaleax commented Dec 22, 2015

rvagg commented Dec 23, 2015

Fishrock123 commented Dec 23, 2015

trevnorris commented Dec 23, 2015

addaleax commented Dec 25, 2015

Fishrock123 commented Dec 25, 2015

addaleax commented Dec 25, 2015

trevnorris commented Dec 27, 2015

cjihrig commented Dec 30, 2015

rvagg commented Jan 4, 2016

addaleax commented Jan 4, 2016

jasnell commented Jan 4, 2016

timers: Fail early when callback is not a function #4362

timers: Fail early when callback is not a function #4362

Conversation

addaleax commented Dec 20, 2015

Fishrock123 commented Dec 21, 2015

cjihrig Dec 21, 2015

Choose a reason for hiding this comment

addaleax commented Dec 21, 2015

Fishrock123 commented Dec 21, 2015

cjihrig commented Dec 22, 2015

Fishrock123 commented Dec 22, 2015

mscdex commented Dec 22, 2015

trevnorris commented Dec 22, 2015

addaleax commented Dec 22, 2015

rvagg commented Dec 23, 2015

Fishrock123 commented Dec 23, 2015

trevnorris commented Dec 23, 2015

addaleax commented Dec 25, 2015

Fishrock123 commented Dec 25, 2015

addaleax commented Dec 25, 2015

trevnorris commented Dec 27, 2015

cjihrig commented Dec 30, 2015

rvagg commented Jan 4, 2016

addaleax commented Jan 4, 2016

jasnell commented Jan 4, 2016