timers: use consistent checks for canceled timers #9685

Fishrock123 · 2016-11-18T20:53:51Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

timers

Description of change

Previously not all codepaths set timer._idleTimeout = -1 for canceled
or closed timers, and not all codepaths checked it either.

Unenroll uses this to say that a timer is indeed closed and it is the
closest thing there is to an authoritative source for this.

Refs: #9606
Fixes: #9561

I think the problem originated from or became more apparent after c8c2544

CI: https://ci.nodejs.org/job/node-test-pull-request/4898/

cc @cjihrig, @misterdjules, @watson

(This patch was made live during https://www.twitch.tv/nodesource/v/101846332 if you'd like to see me working on this in retrospect. :P)

Trott · 2016-11-19T16:16:59Z

test/parallel/test-timers-unenroll-unref-interval.js

+
+const keepOpen = setTimeout(() => {
+  common.fail('Test timed out.');
+}, common.platformTimeout(500)); // Keep event loop open.


~~I would much prefer this just keep the event loop open indefinitely and not throw:~~

~~const keepOpen = setInterval(() => {}, 9999);~~

~~(Would need to change clearTimeout() below to clearInterval().)~~

EDIT: Meh, forget indefinitely, better for just one tick, see below.

@nodejs/testing

@Trott I'm not clear on what benefit that would have, could you elaborate?

All the unref timers are guaranteed to fire before this, it could be just 2ms theoretically in that respect, but we do need to actually keep it open long enough for the 1ms timeouts to happen twice IIRC.

All the unref timers are guaranteed to fire before this,

No, they are not. On a sufficiently heavily loaded machine, they will fail to fire before the common.fail(). If recent history is any guide, this test will be flaky on FreeBSD in CI.

Try this to see it fail (unless you have an absurdly well-provisioned machine):

tools/test.py -j 96 parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-intervalparallel/test-timers-unenroll-unref-interval parallel/test-timers-unenroll-unref-interval

I guess I'll also add/clarify that even If it seems like I'm being ridiculous and this will never ever cause problems itself, arbitrary-length timeouts in tests are a bit of an anti-pattern that have bitten us over and over. So even if it's just to set a good example for other people writing tests, it's good (IMO) to remove arbitrary-length timers.

(And last: I don't object to this landing as-is.)

That is why I suggested what I did in my last comment.

Which suggestion was that?

then there is a very bad ordering bug in the timers impl when under load.

I don't think that's the correct interpretation.

If load is sufficient, the initial setTimeout() callback will fire on the same tick as all the setIntervals() callbacks (although after they run). The setImmediate() callbacks set inside the intervals will happen on the next tick, after the setTimeout() callback has thrown an error.

All the unref timers are guaranteed to fire before this,

No, they are not.

Clarification: Yes, the unref'ed intervals will fire before the timer. However, the test will still fail if the immediates wrapped inside the unref'ed intervals don't fire before the timer, and that's what's not guaranteed.

I don't think that's the correct interpretation.

Would it help if I set up a meeting with you to explain what actually happens with timers and why I am asserting that would be broken behavior?

Clarification: Yes, the unref'ed intervals will fire before the timer. However, the test will still fail if the immediates wrapped inside the unref'ed intervals don't fire before the timer, and that's what's not guaranteed.

Thanks, I'll fix this so that it more precisely relies on the event loop specifics.

Thanks, I'll fix this so that it more precisely relies on the event loop specifics.

Here's what I'd propose, FWIW:

'use strict'; const common = require('../common'); const timers = require('timers'); setImmediate(common.mustCall(() => {})); // Keep event loop open for one tick. { const interval = setInterval(common.mustCall(() => { clearTimeout(interval); setImmediate(common.mustCall(() => {})); }), 1).unref(); } { const interval = setInterval(common.mustCall(() => { interval.close(); setImmediate(common.mustCall(() => {})); }), 1).unref(); } { const interval = setInterval(common.mustCall(() => { timers.unenroll(interval); setImmediate(common.mustCall(() => {})); }), 1).unref(); } { const interval = setInterval(common.mustCall(() => { interval._idleTimeout = -1; setImmediate(common.mustCall(() => {})); }), 1).unref(); } { const interval = setInterval(common.mustCall(() => { interval._onTimeout = null; setImmediate(common.mustCall(() => {})); }), 1).unref(); }

watson · 2016-11-19T16:45:42Z

Thanks @Fishrock123 😍 And I'm on Twich! (sort of) I made it to the big times 😜

Fishrock123 · 2016-11-21T21:38:31Z

Updated. I ran it under much more extreme load without issue. (Almost 1k at 1k parallization.)

New CI: https://ci.nodejs.org/job/node-test-pull-request/4926/console

@Trott could you please read the comment in the test and let me know if it makes sense?

Fishrock123 · 2016-11-21T21:41:06Z

Updated with an extra comment link, CI again: https://ci.nodejs.org/job/node-test-pull-request/4927/

Trott · 2016-11-21T21:51:54Z

@Trott could you please read the comment in the test and let me know if it makes sense?

Comment and test LGTM (assuming no issues discovered in CI, of course).

Trott · 2016-11-21T22:08:11Z

test/parallel/test-timers-unenroll-unref-interval.js

+// than the previous timeouts, unrefed or not.
+//
+// Keep the event loop alive for one timeout and then
+// another. Any problems will ocurr when the second


Previously not all codepaths set `timer._idleTimeout = -1` for canceled or closed timers, and not all codepaths checked it either. Unenroll uses this to say that a timer is indeed closed and it is the closest thing there is to an authoritative source for this. Refs: nodejs#9606 Fixes: nodejs#9561 PR-URL: nodejs#9685 Reviewed-By: Rich Trott <rtrott@gmail.com>

Previously not all codepaths set `timer._idleTimeout = -1` for canceled or closed timers, and not all codepaths checked it either. Unenroll uses this to say that a timer is indeed closed and it is the closest thing there is to an authoritative source for this. Refs: #9606 Fixes: #9561 PR-URL: #9685 Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins · 2016-12-20T19:24:14Z

@Fishrock123 should this be backported to v6?

Fishrock123 · 2016-12-20T23:23:55Z

@thealphanerd This should be backported to both. The test aborts on both LTS versions.

Fishrock123 · 2016-12-20T23:33:16Z

I'm working on a 4.x backport.

Previously not all codepaths set `timer._idleTimeout = -1` for canceled or closed timers, and not all codepaths checked it either. Unenroll uses this to say that a timer is indeed closed and it is the closest thing there is to an authoritative source for this. Refs: nodejs#9606 Fixes: nodejs#9561 PR-URL: nodejs#9685 Reviewed-By: Rich Trott <rtrott@gmail.com> Conflicts: lib/timers.js

Fishrock123 · 2016-12-21T00:06:10Z

@thealphanerd lands on v4.x cleanly, v4.x backport at #10365

joyeecheung · 2016-12-21T07:56:53Z

#4303 looks similar to this one and is still open.

Previously not all codepaths set `timer._idleTimeout = -1` for canceled or closed timers, and not all codepaths checked it either. Unenroll uses this to say that a timer is indeed closed and it is the closest thing there is to an authoritative source for this. Refs: #9606 Fixes: #9561 PR-URL: #9685 Reviewed-By: Rich Trott <rtrott@gmail.com> Conflicts: lib/timers.js

Previously not all codepaths set `timer._idleTimeout = -1` for canceled or closed timers, and not all codepaths checked it either. Unenroll uses this to say that a timer is indeed closed and it is the closest thing there is to an authoritative source for this. Refs: #9606 Fixes: #9561 PR-URL: #9685 Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: #10365 Ref: #9685 Reviewed-By: Myles Borins <myles.borins@gmail.com>

Previously not all codepaths set `timer._idleTimeout = -1` for canceled or closed timers, and not all codepaths checked it either. Unenroll uses this to say that a timer is indeed closed and it is the closest thing there is to an authoritative source for this. Refs: #9606 Fixes: #9561 PR-URL: #9685 Reviewed-By: Rich Trott <rtrott@gmail.com>

Previously not all codepaths set `timer._idleTimeout = -1` for canceled or closed timers, and not all codepaths checked it either. Unenroll uses this to say that a timer is indeed closed and it is the closest thing there is to an authoritative source for this. Refs: #9606 Fixes: #9561 PR-URL: #9685 Reviewed-By: Rich Trott <rtrott@gmail.com> Conflicts: lib/timers.js

PR-URL: #10365 Ref: #9685 Reviewed-By: Myles Borins <myles.borins@gmail.com>

This LTS release comes with 180 commits. This includes 117 which are test related, 34 which are doc related, 15 which are build / tool related, and 1 commit which is an update to dependencies. Notable Changes: * build: - shared library support is now working for AIX builds (Stewart Addison) #9675 * repl: - Passing options to the repl will no longer overwrite defaults (cjihrig) #7826 * timers: - Re canceling a cancelled timers will no longer throw (Jeremiah Senkpiel) #9685 PR-URL: #10395

This LTS release comes with 312 commits. This includes 229 that are test related, 62 that are docs related, 17 which are build / tools related, and 4 commits which are updates to dependencies. Notable Changes: * build: - shared library support is now working for AIX builds (Stewart Addison) #9675 * deps: - *npm*: upgrade npm to 3.10.10 (Rebecca Turner) #9847 - *V8*: Destructuring of arrow function arguments via computed property no longer throws (Michaël Zasso) #10386) * inspector: - /json/version returns object, not an object wrapped in an array (Ben Noordhuis) #9762 * module: - using --debug-brk and --eval together now works as expected (Kelvin Jin) #8876 * process: - improve performance of nextTick up to 20% (Evan Lucas) #8932 * repl: - the division operator will no longer be accidentally parsed as regex (Teddy Katz) #10103 - improved support for generator functions (Teddy Katz) #9852 * timers: - Re canceling a cancelled timers will no longer throw (Jeremiah Senkpiel) #9685 PR-URL: #10394

This LTS release comes with 180 commits. This includes 117 which are test related, 34 which are doc related, 15 which are build / tool related, and 1 commit which is an update to dependencies. Notable Changes: * build: - shared library support is now working for AIX builds (Stewart Addison) #9675 * repl: - Passing options to the repl will no longer overwrite defaults (cjihrig) #7826 * timers: - Re canceling a cancelled timers will no longer throw (Jeremiah Senkpiel) #9685 PR-URL: #10395

This LTS release comes with 180 commits. This includes 117 which are test related, 34 which are doc related, 15 which are build / tool related, and 1 commit which is an update to dependencies. Notable Changes: * build: - shared library support is now working for AIX builds (Stewart Addison) nodejs/node#9675 * repl: - Passing options to the repl will no longer overwrite defaults (cjihrig) nodejs/node#7826 * timers: - Re canceling a cancelled timers will no longer throw (Jeremiah Senkpiel) nodejs/node#9685 PR-URL: nodejs/node#10395 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

This LTS release comes with 312 commits. This includes 229 that are test related, 62 that are docs related, 17 which are build / tools related, and 4 commits which are updates to dependencies. Notable Changes: * build: - shared library support is now working for AIX builds (Stewart Addison) nodejs/node#9675 * deps: - *npm*: upgrade npm to 3.10.10 (Rebecca Turner) nodejs/node#9847 - *V8*: Destructuring of arrow function arguments via computed property no longer throws (Michaël Zasso) nodejs/node#10386) * inspector: - /json/version returns object, not an object wrapped in an array (Ben Noordhuis) nodejs/node#9762 * module: - using --debug-brk and --eval together now works as expected (Kelvin Jin) nodejs/node#8876 * process: - improve performance of nextTick up to 20% (Evan Lucas) nodejs/node#8932 * repl: - the division operator will no longer be accidentally parsed as regex (Teddy Katz) nodejs/node#10103 - improved support for generator functions (Teddy Katz) nodejs/node#9852 * timers: - Re canceling a cancelled timers will no longer throw (Jeremiah Senkpiel) nodejs/node#9685 PR-URL: nodejs/node#10394 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

This LTS release comes with 180 commits. This includes 117 which are test related, 34 which are doc related, 15 which are build / tool related, and 1 commit which is an update to dependencies. Notable Changes: * build: - shared library support is now working for AIX builds (Stewart Addison) nodejs/node#9675 * repl: - Passing options to the repl will no longer overwrite defaults (cjihrig) nodejs/node#7826 * timers: - Re canceling a cancelled timers will no longer throw (Jeremiah Senkpiel) nodejs/node#9685 PR-URL: nodejs/node#10395 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

This LTS release comes with 312 commits. This includes 229 that are test related, 62 that are docs related, 17 which are build / tools related, and 4 commits which are updates to dependencies. Notable Changes: * build: - shared library support is now working for AIX builds (Stewart Addison) nodejs/node#9675 * deps: - *npm*: upgrade npm to 3.10.10 (Rebecca Turner) nodejs/node#9847 - *V8*: Destructuring of arrow function arguments via computed property no longer throws (Michaël Zasso) nodejs/node#10386) * inspector: - /json/version returns object, not an object wrapped in an array (Ben Noordhuis) nodejs/node#9762 * module: - using --debug-brk and --eval together now works as expected (Kelvin Jin) nodejs/node#8876 * process: - improve performance of nextTick up to 20% (Evan Lucas) nodejs/node#8932 * repl: - the division operator will no longer be accidentally parsed as regex (Teddy Katz) nodejs/node#10103 - improved support for generator functions (Teddy Katz) nodejs/node#9852 * timers: - Re canceling a cancelled timers will no longer throw (Jeremiah Senkpiel) nodejs/node#9685 PR-URL: nodejs/node#10394 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

Fishrock123 added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Nov 18, 2016

nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Nov 18, 2016

Trott reviewed Nov 19, 2016

View reviewed changes

Fishrock123 force-pushed the fix-timers-cancel-in-interval branch from c11e465 to 11bb6f2 Compare November 21, 2016 21:36

Fishrock123 force-pushed the fix-timers-cancel-in-interval branch from 11bb6f2 to a89a08f Compare November 21, 2016 21:40

Trott reviewed Nov 21, 2016

View reviewed changes

Fishrock123 force-pushed the fix-timers-cancel-in-interval branch from a89a08f to 55efbfe Compare November 22, 2016 17:24

Fishrock123 force-pushed the fix-timers-cancel-in-interval branch from 55efbfe to 98fea5b Compare November 22, 2016 17:24

Fishrock123 force-pushed the fix-timers-cancel-in-interval branch from 98fea5b to 3f1e38c Compare November 22, 2016 17:28

Fishrock123 merged commit 3f1e38c into nodejs:master Nov 22, 2016

MylesBorins added dont-land-on-v4.x labels Dec 20, 2016

Fishrock123 self-assigned this Dec 20, 2016

Fishrock123 added lts-watch-v4.x and removed dont-land-on-v4.x labels Dec 20, 2016

Fishrock123 mentioned this pull request Dec 21, 2016

Fix timers cancel in interval v4.x #10365

Closed

3 tasks

MylesBorins pushed a commit that referenced this pull request Dec 21, 2016

timers: bail from intervals if _repeat is bad

759e8fd

PR-URL: #10365 Ref: #9685 Reviewed-By: Myles Borins <myles.borins@gmail.com>

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Dec 21, 2016

MylesBorins pushed a commit that referenced this pull request Dec 21, 2016

timers: bail from intervals if _repeat is bad

0be437a

PR-URL: #10365 Ref: #9685 Reviewed-By: Myles Borins <myles.borins@gmail.com>

This was referenced Dec 21, 2016

V6.9.3 proposal #10394

Merged

V4.7.1 proposal #10395

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

timers: use consistent checks for canceled timers #9685

timers: use consistent checks for canceled timers #9685

Fishrock123 commented Nov 18, 2016 •

edited

Loading

Trott Nov 19, 2016 •

edited

Loading

Fishrock123 Nov 19, 2016

Trott Nov 19, 2016

Trott Nov 20, 2016

Trott Nov 20, 2016

Trott Nov 21, 2016

Trott Nov 21, 2016 •

edited

Loading

Trott Nov 21, 2016

Fishrock123 Nov 21, 2016

Trott Nov 21, 2016 •

edited

Loading

watson commented Nov 19, 2016

Fishrock123 commented Nov 21, 2016

Fishrock123 commented Nov 21, 2016

Trott commented Nov 21, 2016 •

edited

Loading

Trott Nov 21, 2016

MylesBorins commented Dec 20, 2016

Fishrock123 commented Dec 20, 2016

Fishrock123 commented Dec 20, 2016

Fishrock123 commented Dec 21, 2016

joyeecheung commented Dec 21, 2016

timers: use consistent checks for canceled timers #9685

timers: use consistent checks for canceled timers #9685

Conversation

Fishrock123 commented Nov 18, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

Trott Nov 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott Nov 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott Nov 21, 2016 • edited Loading

Choose a reason for hiding this comment

watson commented Nov 19, 2016

Fishrock123 commented Nov 21, 2016

Fishrock123 commented Nov 21, 2016

Trott commented Nov 21, 2016 • edited Loading

Choose a reason for hiding this comment

MylesBorins commented Dec 20, 2016

Fishrock123 commented Dec 20, 2016

Fishrock123 commented Dec 20, 2016

Fishrock123 commented Dec 21, 2016

joyeecheung commented Dec 21, 2016

Fishrock123 commented Nov 18, 2016 •

edited

Loading

Trott Nov 19, 2016 •

edited

Loading

Trott Nov 21, 2016 •

edited

Loading

Trott Nov 21, 2016 •

edited

Loading

Trott commented Nov 21, 2016 •

edited

Loading