timers: fix regression with clearImmediate() #9086

mscdex · 2016-10-13T22:07:36Z

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

This commit fixes a regression introduced in 0ed8839 that caused additional queued immediate callbacks to be ignored if clearImmediate(immediate) was called within the callback for immediate.

Fixes: #9084

CI: https://ci.nodejs.org/job/node-test-pull-request/4513/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/415/

Trott · 2016-10-13T22:41:32Z

I think the test can be sped up and simplified like this:

'use strict';
require('../common');
const assert = require('assert');

const jobs = 5;
var count = 0;

for (var i = 0; i < jobs; i++) {
  const timeout = setTimeout(() => {
    clearTimeout(timeout);

    (function retry() {
      var immediate = setImmediate(() => {
        clearImmediate(immediate);

        ++count;
      });
    }());
  }, 0);
}

process.on('exit', () => {
  assert.strictEqual(count, jobs);
});

This fails reliably for me with v6.8.0 and passes reliably with v6.7.0.

mscdex · 2016-10-13T22:44:08Z

I've reduced the test to something even simpler, as it really has nothing to do with setTimeout()/clearTimeout().

Trott · 2016-10-13T22:44:42Z

test/parallel/test-timers-clearImmediate.js

+const assert = require('assert');
+
+var count = 0;
+const timestamp = Date.now();


This constant appears to be unused.

(And before anybody asks, I just checked, and yes, the linter catches it.)

Too late, I already removed it ;-)

Trott · 2016-10-13T22:49:05Z

The removal of setTimeout() entirely also seems to resolve some non-determinancy I was seeing where the test would usually fail with n of 1, but every once in a while, n would be 2. 🎉

Trott

LGTM if there are no surprises lurking in CI.

Since this is a fix of a bug introduced in a change that brought a performance improvement, is it at all worthwhile to run the same benchmark that was run on the original change just to make sure there's no surprises there? (This change seems to me to be unlikely to cause a performance regression, but what do I know?)

Fishrock123 · 2016-10-13T22:54:48Z

test/parallel/test-timers-clearImmediate.js

+  next();
+
+process.on('exit', () => {
+  assert.strictEqual(count, 3);


a failure description (3rd argument) would be nice

Fishrock123 · 2016-10-13T22:55:53Z

lib/timers.js

@@ -696,6 +705,7 @@ function createImmediate(args, callback) {


 exports.clearImmediate = function(immediate) {
+  // TODO: also verify `immediate instanceof Immediate`?


the linkedlist remove should already be guarding against problems, seems unnecessary

It only checks property names, which I think could be problematic if someone passes in something that isn't a value returned by setImmediate()? I've removed the comment for now anyway...

hmmm you might be right, probably not for this pr tho

Fishrock123 · 2016-10-13T22:56:30Z

lib/timers.js

@@ -575,12 +575,21 @@ function processImmediate() {
      domain.enter();

    immediate._callback = immediate._onImmediate;
+
+    // Save next in case `clearImmediate(immediate)` is called from callback
+    var next = immediate._idleNext;


I kept with var because it is still used throughout timers.js.

Fishrock123 · 2016-10-13T22:57:14Z

test/parallel/test-timers-clearImmediate.js

+  });
+}
+for (var i = 0; i < 3; ++i)
+  next();


wasn't the original test recursive? should this still be?

It wasn't really recursive, just "restarting" an immediate timer. However, that's not necessary to reproduce the original issue. The issue just has to do with calling clearImmediate() from within an immediate callback when there are other items in the immediate queue.

mscdex · 2016-10-13T23:02:18Z

@Trott I already checked, there is no performance regression with this fix.

This commit fixes a regression introduced in 0ed8839 that caused additional queued immediate callbacks to be ignored if `clearImmediate(immediate)` was called within the callback for `immediate`. Fixes: nodejs#9084

Trott · 2016-10-13T23:34:02Z

test/parallel/test-timers-clearImmediate.js

+  next();
+
+process.on('exit', () => {
+  assert.strictEqual(count, N, `Expected ${N} immediate callback executions`);


Nit: Maybe even go a bit further?

'use strict'; const common = require('../common'); function next() { const immediate = setImmediate(common.mustCall(function callback() { clearImmediate(immediate); })); } for (var i = 0; i < 3; ++i) next();

One benefit is that we don't lose the information of what count was if the assertion fires.

One benefit is that we don't lose the information of what count was if the assertion fires.

I don't understand what you mean here.

With the way the test is written now, when the test fails, we get:

AssertionError: Expected 3 immediate callback executions

So, we were expecting 3, but we got some other number. How many? 0? 1? 2? 4?

So I was looking at how to work count in there so that information would be preserved, but I realized we could just do away with the explicit exit listener entirely if we use common.mustCall(). So if you use the test as written in that previous comment and it fails, you get results of this format:

Mismatched callback function calls. Expected 1, actual 0.

And the test is a compact 10 lines to boot.

(As usual: It's a nit, so you know, ignore my suggestion if you don't like it.)

@Fishrock123 Is this ok with you, since you previously requested that a custom assertion message be added?

Fishrock123

lgtm

jasnell

LGTM

evanlucas

LGTM

evanlucas · 2016-10-14T21:05:29Z

Landed in 42158a0. Thanks!

This commit fixes a regression introduced in 0ed8839 that caused additional queued immediate callbacks to be ignored if `clearImmediate(immediate)` was called within the callback for `immediate`. PR-URL: #9086 Fixes: #9084 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

* build: Fix building with shared zlib. (Bradley T. Hughes) [#9077](#9077) * stream: fix `Writable` subclass instanceof checks (Anna Henningsen) [#9088](#9088) * timers: fix regression with clearImmediate() (Brian White) [#9086](#9086) PR-URL: #9104

This commit fixes a regression introduced in 0ed8839 that caused additional queued immediate callbacks to be ignored if `clearImmediate(immediate)` was called within the callback for `immediate`. PR-URL: #9086 Fixes: #9084 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

* build: Fix building with shared zlib. (Bradley T. Hughes) [#9077](nodejs/node#9077) * stream: fix `Writable` subclass instanceof checks (Anna Henningsen) [#9088](nodejs/node#9088) * timers: fix regression with clearImmediate() (Brian White) [#9086](nodejs/node#9086) Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

MylesBorins · 2016-11-22T06:43:20Z

@Fishrock123 should this be bacporrted?

mscdex · 2016-11-22T06:46:55Z

@thealphanerd The original PR (#8655) these changes depend on was never landed on v4.x, so I would say no.

However, the original PR is also marked as lts-watch-v4.x, so both would need to be backported if the original is backported.

mscdex added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Oct 13, 2016

mscdex force-pushed the timers-fix-setImmediate-regression branch from f1481ac to 2f4bc27 Compare October 13, 2016 22:08

mscdex mentioned this pull request Oct 13, 2016

setImmediate regression in v6.8.0 #9084

Closed

Fishrock123 mentioned this pull request Oct 13, 2016

v6.8.1 patch release? #9087

Closed

mscdex force-pushed the timers-fix-setImmediate-regression branch from 2f4bc27 to 8d0d489 Compare October 13, 2016 22:43

Trott reviewed Oct 13, 2016

View reviewed changes

mscdex force-pushed the timers-fix-setImmediate-regression branch from 8d0d489 to 46f9d1f Compare October 13, 2016 22:45

Trott approved these changes Oct 13, 2016

View reviewed changes

Fishrock123 reviewed Oct 13, 2016

View reviewed changes

mscdex force-pushed the timers-fix-setImmediate-regression branch from 46f9d1f to 29a03ae Compare October 13, 2016 23:04

timers: fix regression with clearImmediate()

6750abf

This commit fixes a regression introduced in 0ed8839 that caused additional queued immediate callbacks to be ignored if `clearImmediate(immediate)` was called within the callback for `immediate`. Fixes: nodejs#9084

mscdex force-pushed the timers-fix-setImmediate-regression branch from 29a03ae to 6750abf Compare October 13, 2016 23:04

Trott reviewed Oct 13, 2016

View reviewed changes

Fishrock123 approved these changes Oct 14, 2016

View reviewed changes

jasnell approved these changes Oct 14, 2016

View reviewed changes

evanlucas approved these changes Oct 14, 2016

View reviewed changes

evanlucas closed this Oct 14, 2016

mscdex deleted the timers-fix-setImmediate-regression branch October 14, 2016 21:34

evanlucas mentioned this pull request Oct 14, 2016

v6.8.1 proposal #9104

Merged

MylesBorins added the lts-watch-v4.x label Nov 22, 2016

aliscco mentioned this pull request Nov 29, 2023

[Snyk] Security upgrade eslint from 2.13.1 to 4.5.0 aliscco/alisco-node#705

Open

leonardoadame mentioned this pull request Nov 29, 2023

[Snyk] Fix for 4 vulnerabilities leonardoadame/node#246

Open

This was referenced Nov 30, 2023

[Snyk] Security upgrade eslint from 2.13.1 to 4.5.0 aliscco/alisco-node#826

Open

[Snyk] Fix for 3 vulnerabilities aliscco/alisco-node#846

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

timers: fix regression with clearImmediate() #9086

timers: fix regression with clearImmediate() #9086

mscdex commented Oct 13, 2016 •

edited

Loading

Trott commented Oct 13, 2016 •

edited

Loading

mscdex commented Oct 13, 2016

Trott Oct 13, 2016

Trott Oct 13, 2016

mscdex Oct 13, 2016

Trott commented Oct 13, 2016 •

edited

Loading

Trott left a comment

Fishrock123 Oct 13, 2016

mscdex Oct 13, 2016

Fishrock123 Oct 13, 2016

mscdex Oct 13, 2016

Fishrock123 Oct 14, 2016

Fishrock123 Oct 13, 2016

mscdex Oct 13, 2016 •

edited

Loading

Fishrock123 Oct 13, 2016

mscdex Oct 13, 2016 •

edited

Loading

mscdex commented Oct 13, 2016 •

edited

Loading

Trott Oct 13, 2016

mscdex Oct 13, 2016

Trott Oct 14, 2016

mscdex Oct 14, 2016 •

edited

Loading

Fishrock123 left a comment

jasnell left a comment

evanlucas left a comment

evanlucas commented Oct 14, 2016

MylesBorins commented Nov 22, 2016

mscdex commented Nov 22, 2016 •

edited

Loading

		@@ -696,6 +705,7 @@ function createImmediate(args, callback) {


		exports.clearImmediate = function(immediate) {
		// TODO: also verify `immediate instanceof Immediate`?

timers: fix regression with clearImmediate() #9086

timers: fix regression with clearImmediate() #9086

Conversation

mscdex commented Oct 13, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

Trott commented Oct 13, 2016 • edited Loading

mscdex commented Oct 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Oct 13, 2016 • edited Loading

Trott left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

mscdex commented Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Oct 14, 2016 • edited Loading

Choose a reason for hiding this comment

Fishrock123 left a comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

evanlucas left a comment

Choose a reason for hiding this comment

evanlucas commented Oct 14, 2016

MylesBorins commented Nov 22, 2016

mscdex commented Nov 22, 2016 • edited Loading

mscdex commented Oct 13, 2016 •

edited

Loading

Trott commented Oct 13, 2016 •

edited

Loading

Trott commented Oct 13, 2016 •

edited

Loading

mscdex Oct 13, 2016 •

edited

Loading

mscdex Oct 13, 2016 •

edited

Loading

mscdex commented Oct 13, 2016 •

edited

Loading

mscdex Oct 14, 2016 •

edited

Loading

mscdex commented Nov 22, 2016 •

edited

Loading