timers: prevent infinite loop when subsequent immediates are cleared #9759

hassy · 2016-11-23T11:52:52Z

PR to go with: #9756

I am not familiar with timers, so the proposed solution could well be wrong. Looking forward to feedback on that!

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

Remove the unnecessary if branch.

If current immediate has no callback, proceed onto the next one in the queue.

italoacasas · 2016-11-23T14:07:16Z

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

cc: @Fishrock123

sam-github · 2016-11-23T14:25:58Z

You should run make test to check for regressions and add a test. /cc @mscdex

Fishrock123 · 2016-11-23T14:57:40Z

lib/timers.js

@@ -580,13 +580,15 @@ function processImmediate() {
  while (immediate) {
    domain = immediate.domain;

-    if (!immediate._onImmediate)
-      continue;


This doesn't seem correct. We should definitely always skip to the next here if there is no callback (_onImmediate).

Would it work if you moved https://github.com/nodejs/node/pull/9759/files#diff-0a5d4868b2b9b17cf9e2c11f1bd1311eR594 above this?

Not sure, but I think that would only protect against the very next immediate being cleared in the callback, and not anything else after the next one in the queue and we'd be back in an infinite loop? I'll run some tests!

Yep, it seems moving those lines above the if branch has no effect, we still get stuck.

@hassy Could you try re-running the tests on your machine with the following patch applied?

When I tried moving the conditional above, it ran fine locally.

diff --git a/lib/timers.js b/lib/timers.js index 37ac317..d89a1d8 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -578,17 +578,18 @@ function processImmediate() { immediateQueue.head = immediateQueue.tail = null; while (immediate) { + + if (!immediate._onImmediate) { + immediate = immediate._idleNext; + continue; + } + domain = immediate.domain; if (domain) domain.enter(); - if (immediate._onImmediate) { - immediate._callback = immediate._onImmediate; - } else { - immediate = immediate._idleNext; - continue; - } + immediate._callback = immediate._onImmediate; // Save next in case `clearImmediate(immediate)` is called from callback var next = immediate._idleNext;

Yep, that works (since we are moving onto the next immediate before continueing rather than just continuing as in the original if branch, which is what led to the infinite loop).

I think I misunderstood your first comment above and tested the wrong thing.

Fishrock123 · 2016-11-23T14:58:13Z

@sam-github The OP indicated that he tried to.

Fishrock123 · 2016-11-23T15:00:03Z

@hassy What tests fail for you on OS X? What OS X version? (I use OS X and they pass for me with this commit.)

Also, we have a handy guide for writing node core tests, once you are ready. :)

hassy · 2016-11-23T15:34:16Z

Thanks @Fishrock123. I have added a regression test for the bug now - does that look OK?

Fishrock123

Looking better, please run make -j4 lint though. :)

Fishrock123 · 2016-11-23T15:41:57Z

test/sequential/test-regress-GH-9765.js

+// This test ensures that if an Immediate callback clears subsequent
+// immediates we don't get stuck in an infinite loop.
+//
+// Ref: https://github.com/nodejs/node/issues/9756


This probably isn't needed, the test runner has a timeout mechanism of it's own.

If you think it is, the other file should be in /fixtures/.

Either way, this file should probably be in /parallel/.

Fishrock123 · 2016-11-23T15:42:20Z

test/sequential/test-regress-GH-9765.js

+setTimeout(function killProcess() {
+  // Presume the child process is spinning at 100% CPU and won't exit
+  childProcess.kill();
+  assert(false, 'Child process didn\'t exit in time');


common.fail(message)

Fishrock123 · 2016-11-23T15:42:49Z

test/sequential/test-regress-GH-9765.js

+const childProcess = fork(path.join(__dirname, 'set-and-clear-immediates.js'));
+
+childProcess.on('close', function(code) {
+  assert(true);


does nothing, instead, wrap the callback in common.mustCall().

hassy · 2016-11-23T16:13:41Z

Thanks @Fishrock123! I replaced the test with a smaller one, in the right place, and linted too. :)

hassy · 2016-11-24T10:31:55Z

Don't think any documentation needs to be updated for this? If the fix & the test look OK now, I'll squash the commits and write a proper commit message.

hassy · 2016-12-05T14:28:18Z

@mscdex would you mind reviewing the patch please? I think you have the final say on whether this is OK or not. :-)

Fishrock123 · 2016-12-09T19:35:37Z

test/parallel/test-timers-regress-GH-9765.js

+});
+
+const i2 = setImmediate(function() {
+});


Tiny nit: could you put a common.fail(<message>) into each of these? Thanks!

Fishrock123 · 2016-12-09T19:38:08Z

test/parallel/test-timers-regress-GH-9765.js

+//
+// Ref: https://github.com/nodejs/node/issues/9756
+
+setImmediate(function() {


Could you also wrap this with common.mustCall()? :D

Fishrock123 · 2016-12-09T19:41:12Z

@hassy I've left a few comments (including one up above regarding the conditional placement), if you could fix these up we should be good to merge, I think.

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

Fishrock123 · 2016-12-12T19:08:17Z

@hassy Ping? :D

hassy · 2016-12-12T19:35:58Z

@Fishrock123 Thanks! Didn't get a notification on GH for some reason. I'll update the PR with the fixes.

If current immediate has no callback, move on to the next one in the queue. Fixes: nodejs#9756 PR-URL: nodejs#9759

hassy · 2016-12-13T13:10:18Z

@Fishrock123 Is it OK to git push --force-with-lease with the final fixes and a proper commit message as per CONTRIBUTING.md?

Fishrock123 · 2016-12-13T17:25:00Z

@hassy yes, we force push all the time to update PRs. Even just --force is fine.

Feel free to squash the commits while you are at it too. :)

hassy · 2016-12-13T17:57:10Z

@Fishrock123 Cool, done. :-)

Fishrock123

Looks good to me. New CI: https://ci.nodejs.org/job/node-test-pull-request/5384/

Fishrock123 · 2016-12-13T21:48:12Z

@hassy btw your git info says your name is hveldstra <h@veldstra.org>

If you would like to update it to a name you prefer more, you can update it like so:

$ git config --global user.email "j.random.user@example.com"
$ git config --global user.name "J. Random User"

Then

$ git commit --amend --reset-author
$ git push origin head --force

Otherwise, just let us know that hveldstra is what you'd prefer to be known as in the commit history / Author file. :)

Good to land otherwise. Sorry for the delay again.

hassy · 2016-12-14T09:16:34Z

@Fishrock123 hveldstra is fine. Thanks for all of your help with this PR!

Fishrock123 · 2016-12-14T15:12:59Z

Ok cool. Thanks for the contribution! Landed in 9f6f0f7 :D

If current immediate has no callback, move on to the next one in the queue. Fixes: #9756 PR-URL: #9759 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Notable changes SEMVER-MINOR - url: - add inspect function to TupleOrigin (Safia Abdalla) #10039 - crypto: - allow adding extra certs to well-known CAs (Sam Roberts) #9139 SEMVER-PATCH - buffer: - fix single-character string filling (Anna Henningsen) #9837 - handle UCS2 .fill() properly on BE (Anna Henningsen) #9837 - url: - including base argument in originFor (joyeecheung) #10021 - improve URLSearchParams spec compliance (Timothy Gu) #9484 - http: - remove stale timeout listeners (Karl Böhlmark) #9440 - build: - fix node_g target (Daniel Bevenius) #10153 - fs: - remove unused argument from copyObject() (Ethan Arrowood) #10041 - timers: - fix handling of cleared immediates (hveldstra) #9759 PR-URL: #10277

Notable changes: * **crypto**: - Allow adding extra certificates to well-known CAs. (Sam Roberts) [#9139](#9139) * **buffer**: - Fix single-character string filling. (Anna Henningsen) [#9837](#9837) - Handle UCS2 `.fill()` properly on BE. (Anna Henningsen) [#9837](#9837) * **url**: - Add inspect function to TupleOrigin. (Safia Abdalla) [#10039](#10039) - Including base argument in originFor. (joyeecheung) [#10021](#10021) - Improve URLSearchParams spec compliance. (Timothy Gu) [#9484](#9484) * **http**: - Remove stale timeout listeners. (Karl Böhlmark) [#9440](#9440) * **build**: - Fix node_g target. (Daniel Bevenius) [#10153](#10153) * **fs**: - Remove unused argument from copyObject(). (EthanArrowood) [#10041](#10041) * **timers**: - Fix handling of cleared immediates. (hveldstra) [#9759](#9759) * **src**: - Add wrapper for process.emitWarning(). (SamRoberts) [#9139](#9139) - Fix string format mistake for 32 bit node.(Alex Newman) [#10082](#10082)

If current immediate has no callback, move on to the next one in the queue. Fixes: #9756 PR-URL: #9759 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

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

hassy changed the title ~~timers: prevent infinite loop when subsequent immediates are cleared~~ timers: prevent infinite loop when subsequent immediates are cleared - WIP Nov 23, 2016

Fishrock123 suggested changes Nov 23, 2016

View reviewed changes

hassy changed the title ~~timers: prevent infinite loop when subsequent immediates are cleared - WIP~~ timers: prevent infinite loop when subsequent immediates are cleared Nov 30, 2016

hassy mentioned this pull request Dec 7, 2016

Bug: Artillery won't exit once the test is done artilleryio/artillery#215

Closed

Fishrock123 self-assigned this Dec 8, 2016

Fishrock123 reviewed Dec 9, 2016

View reviewed changes

Fishrock123 suggested changes Dec 9, 2016

View reviewed changes

timers: fix handling of cleared immediates

0db74b5

If current immediate has no callback, move on to the next one in the queue. Fixes: nodejs#9756 PR-URL: nodejs#9759

hassy force-pushed the fix_timers_infinite_loop branch from edee849 to 0db74b5 Compare December 13, 2016 17:55

nodejs-github-bot added the lts-watch-v6.x label Dec 13, 2016

Fishrock123 approved these changes Dec 13, 2016

View reviewed changes

Fishrock123 closed this Dec 14, 2016

Fishrock123 pushed a commit that referenced this pull request Dec 14, 2016

timers: fix handling of cleared immediates

9f6f0f7

If current immediate has no callback, move on to the next one in the queue. Fixes: #9756 PR-URL: #9759 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

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

timers: fix handling of cleared immediates

f664613

If current immediate has no callback, move on to the next one in the queue. Fixes: #9756 PR-URL: #9759 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

italoacasas mentioned this pull request Dec 15, 2016

v7.3.0 proposal #10277

Merged

jcemer mentioned this pull request Dec 27, 2016

Timer issues with node version 7.3.0 jestjs/jest#2450

Closed

MylesBorins added dont-land-on-v4.x and removed lts-watch-v6.x labels Jan 22, 2017

MylesBorins pushed a commit that referenced this pull request Jan 22, 2017

timers: fix handling of cleared immediates

b19f2cf

If current immediate has no callback, move on to the next one in the queue. Fixes: #9756 PR-URL: #9759 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

MylesBorins pushed a commit that referenced this pull request Jan 24, 2017

timers: fix handling of cleared immediates

3605726

If current immediate has no callback, move on to the next one in the queue. Fixes: #9756 PR-URL: #9759 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

MylesBorins mentioned this pull request Jan 24, 2017

v6.10.0 proposal #10974

Merged

MylesBorins pushed a commit that referenced this pull request Jan 31, 2017

timers: fix handling of cleared immediates

b868ce6

If current immediate has no callback, move on to the next one in the queue. Fixes: #9756 PR-URL: #9759 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

timers: prevent infinite loop when subsequent immediates are cleared #9759

timers: prevent infinite loop when subsequent immediates are cleared #9759

hassy commented Nov 23, 2016 •

edited

Loading

italoacasas commented Nov 23, 2016 •

edited

Loading

sam-github commented Nov 23, 2016

Fishrock123 Nov 23, 2016

hassy Nov 23, 2016 •

edited

Loading

hassy Nov 23, 2016

Fishrock123 Dec 9, 2016

hassy Dec 13, 2016

Fishrock123 commented Nov 23, 2016 •

edited

Loading

Fishrock123 commented Nov 23, 2016

hassy commented Nov 23, 2016

Fishrock123 left a comment

Fishrock123 Nov 23, 2016

Fishrock123 Nov 23, 2016

Fishrock123 Nov 23, 2016

hassy commented Nov 23, 2016

hassy commented Nov 24, 2016

hassy commented Dec 5, 2016

Fishrock123 Dec 9, 2016

Fishrock123 Dec 9, 2016

Fishrock123 commented Dec 9, 2016

Fishrock123 commented Dec 12, 2016

hassy commented Dec 12, 2016

hassy commented Dec 13, 2016 •

edited

Loading

Fishrock123 commented Dec 13, 2016

hassy commented Dec 13, 2016

Fishrock123 left a comment

Fishrock123 commented Dec 13, 2016

hassy commented Dec 14, 2016

Fishrock123 commented Dec 14, 2016

timers: prevent infinite loop when subsequent immediates are cleared #9759

timers: prevent infinite loop when subsequent immediates are cleared #9759

Conversation

hassy commented Nov 23, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

italoacasas commented Nov 23, 2016 • edited Loading

sam-github commented Nov 23, 2016

Choose a reason for hiding this comment

hassy Nov 23, 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

Fishrock123 commented Nov 23, 2016 • edited Loading

Fishrock123 commented Nov 23, 2016

hassy commented Nov 23, 2016

Fishrock123 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

hassy commented Nov 23, 2016

hassy commented Nov 24, 2016

hassy commented Dec 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Dec 9, 2016

Fishrock123 commented Dec 12, 2016

hassy commented Dec 12, 2016

hassy commented Dec 13, 2016 • edited Loading

Fishrock123 commented Dec 13, 2016

hassy commented Dec 13, 2016

Fishrock123 left a comment

Choose a reason for hiding this comment

Fishrock123 commented Dec 13, 2016

hassy commented Dec 14, 2016

Fishrock123 commented Dec 14, 2016

hassy commented Nov 23, 2016 •

edited

Loading

italoacasas commented Nov 23, 2016 •

edited

Loading

hassy Nov 23, 2016 •

edited

Loading

Fishrock123 commented Nov 23, 2016 •

edited

Loading

hassy commented Dec 13, 2016 •

edited

Loading