timers: prevent infinite loop when subsequent immediates are cleared by hassy · Pull Request #9759 · nodejs/node · GitHub

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.

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

cc: @Fishrock123

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

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.

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

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

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

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

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

common.fail(message)

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

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

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.

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

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

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

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

@hassy Ping? :D

@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

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

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

@Fishrock123 Cool, done. :-)

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

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

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

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>

timers: prevent infinite loop when subsequent immediates are cleared by hassy · Pull Request #9759 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

timers: prevent infinite loop when subsequent immediates are cleared#9759hassy wants to merge 1 commit intonodejs:masternodejs/node:masterfrom hassy:fix_timers_infinite_loophassy/node:fix_timers_infinite_loopCopy head branch name to clipboard

Conversation

hassy commented Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

italoacasas commented Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sam-github commented Nov 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hassy Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fishrock123 commented Nov 23, 2016

Uh oh!

hassy commented Nov 23, 2016

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hassy commented Nov 23, 2016

Uh oh!

hassy commented Nov 24, 2016

Uh oh!

hassy commented Dec 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Dec 9, 2016

Uh oh!

Fishrock123 commented Dec 12, 2016

Uh oh!

hassy commented Dec 12, 2016

Uh oh!

hassy commented Dec 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fishrock123 commented Dec 13, 2016

Uh oh!

hassy commented Dec 13, 2016

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Dec 13, 2016

Uh oh!

hassy commented Dec 14, 2016

Uh oh!

Fishrock123 commented Dec 14, 2016

Uh oh!

Reviewers

Assignees

Labels

timers: prevent infinite loop when subsequent immediates are cleared#9759
hassy wants to merge 1 commit into
nodejs:masterfrom
hassy:fix_timers_infinite_loop

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