timer,domain: maintain order of timers upon uncaught exception #10522

jBarz · 2016-12-29T16:57:22Z

Checklist

make -j4 test (UNIX)
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

timer, domain, tests

Description of change

The issue in #1271 shows that the callback for timers do not get called in
the order they were created if the domain handles an "uncaught exception"

jasnell · 2016-12-29T17:53:53Z

/cc @Fishrock123

gibfahn · 2016-12-29T21:19:56Z

test/parallel/test-domain-timers-uncaught-exception.js

@@ -0,0 +1,14 @@
+const domain = require('domain').create();
+var first = false;
+


Can't comment much on the other change, but the test needs a 'use strict', a require('../common'), and a little comment explaining what this tests would be nice (though not actually required).

If you haven't seen the guide to writing tests then I recommend reading through it. Also the common.js API, there might be something useful in there.

whoa! Didn't see the test guide. Will follow :-)

sam-github · 2016-12-29T21:21:21Z

test/parallel/test-domain-timers-uncaught-exception.js

+  setTimeout(function() { throw Error('FAIL'); }, 1);
+  setTimeout(function() { first = true; }, 1);
+  setTimeout(function() {
+    if (first === false) {


Why can't you use assert.strictEqual(first, true);? Is it because you are in a domain, and the assert won't work?

my bad. Assert works in the domain. I will change it to use that

sam-github · 2016-12-29T21:22:14Z

test/parallel/test-domain-timers-uncaught-exception.js

+  }, 2);
+});
+
+domain.once('error', function() {});


Shouldn't this be either a common.mustCall(), if it should be called, or an assert.fail if it should not?

ahh yes! It should be called because of the unhandled exception falling through.
Will wrap with common.mustCall.

gibfahn

The test LGTM

gibfahn · 2016-12-30T15:05:04Z

CI: https://ci.nodejs.org/job/node-test-commit/6931/

EDIT: Looks like there's some infra flakyness at the moment.

sam-github · 2016-12-30T15:12:14Z

test/parallel/test-domain-timers-uncaught-exception.js

+
+// This test ensures that the timer callbacks are
+// called in the order in which they were created
+// in the event of an unhandled exception in the domain


punctuate with . and wrap to 80 columns

sam-github · 2016-12-30T15:13:41Z

test/parallel/test-domain-timers-uncaught-exception.js

+domain.run(function() {
+  setTimeout(function() { throw Error('FAIL'); }, 1);
+  setTimeout(function() { first = true; }, 1);
+  setTimeout(function() { assert.strictEqual(first, true);  }, 2);


do es6 functions break the test case? you are using a mixture of es6 and normal js, there is a wave of es6-ization going though the test suite ATM, better catch it and make these es6, or comment on why they must be function

Good catch!. Will make them es6

sam-github · 2016-12-30T15:14:28Z

test/parallel/test-domain-timers-uncaught-exception.js

+  setTimeout(function() { assert.strictEqual(first, true);  }, 2);
+});
+
+domain.once('error', common.mustCall(() => {}));


I assume the err arg here must be that 'FAIL' one, but you could make it explicit

jasnell · 2017-01-06T17:55:22Z

@Fishrock123 ... would definitely appreciate a review and sign off from you on this one

Fishrock123

Fix looks good, test seem inherently flaky though, won't it only ever catch the problem once in a while?

I don't know if there is a good fix tho.

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

Fishrock123 · 2017-01-13T17:14:28Z

lib/timers.js

@@ -239,6 +246,14 @@ function tryOnTimeout(timer, list) {
  } finally {
    if (!threw) return;

+    // Postpone all list events to next tick. We need to do this


all later?

gibfahn · 2017-01-19T12:58:55Z

@Fishrock123 do you think this is good to land as is?

Fishrock123

LGTM other than the test nit.

@jBarz It would be great if you could update this, but if you don't have time I'll update and merge it sometime next week. :)

Fishrock123 · 2017-02-10T20:40:09Z

test/parallel/test-domain-timers-uncaught-exception.js

+  setTimeout(() => { throw new Error('FAIL'); }, 1);
+  setTimeout(() => { first = true; }, 1);
+  setTimeout(() => { assert.strictEqual(first, true); }, 2);
+});


@jBarz sorry for taking a long time to get back to this.

Could you add something in the end of domain.run() that will eat some processor time to ensure that 2ms had always passed and that this test will fail 100% (or 99%) of the time if the patch ever is wrongly changed in the future?

I'm thinking something like the following, which seemed to work just fine (even at 1e5 iterations) locally:

domain.run(function() { // ... let i = 1e6; while (i--); });

jBarz · 2017-02-13T20:01:19Z

@Fishrock123: I have updated this PR with the changes you requested.

Fishrock123

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

Fishrock123 · 2017-02-14T23:55:33Z

@evanlucas the browser extension doesn't like this PR, says some reviewers have not finalized.

Landing manually.

Fishrock123 · 2017-02-15T00:05:57Z

Thanks! Landed in 9dac165! 🎉

PR-URL: #10522 Fixes: #1271 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

PR-URL: nodejs#10522 Fixes: nodejs#1271 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

PR-URL: #10522 Fixes: #1271 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

jasnell · 2017-03-07T17:06:44Z

This would need a backport PR to land in v4 (if it should land there)

PR-URL: #10522 Fixes: #1271 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Notable changes * performance: The performance of several APIs has been improved. - `Buffer.compare()` is up to 35% faster on average. (Brian White) #10927 - `buffer.toJSON()` is up to 2859% faster on average. (Brian White) #10895 - `fs.*statSync()` functions are now up to 9.3% faster on average. (Brian White) #11522 - `os.loadavg` is up to 151% faster. (Brian White) #11516 - `process.memoryUsage()` is up to 34% faster. (Brian White) #11497 - `querystring.unescape()` for `Buffer`s is 15% faster on average. (Brian White) #10837 - `querystring.stringify()` is up to 7.8% faster on average. (Brian White) #10852 - `querystring.parse()` is up to 21% faster on average. (Brian White) #10874 * IPC: - Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) #10677 - Performance gains may be up to 40% for some workloads. * child_process: - `spawnSync` now returns a null `status` when child is terminated by a signal. (cjihrig) #11288 - This fixes the behavior to act like `spawn()` does. * http: - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) #8923 - Debug messages have been added for cases when headers contain invalid values. (Evan Lucas) #9195 * node: - Heap statistics now support values larger than 4GB. (Ben Noordhuis) #10186 * timers: - Timer callbacks now always maintain order when interacting with domain error handling. (John Barboza) #10522 PR-URL: #11759

Notable changes * performance: The performance of several APIs has been improved. - `Buffer.compare()` is up to 35% faster on average. (Brian White) nodejs/node#10927 - `buffer.toJSON()` is up to 2859% faster on average. (Brian White) nodejs/node#10895 - `fs.*statSync()` functions are now up to 9.3% faster on average. (Brian White) nodejs/node#11522 - `os.loadavg` is up to 151% faster. (Brian White) nodejs/node#11516 - `process.memoryUsage()` is up to 34% faster. (Brian White) nodejs/node#11497 - `querystring.unescape()` for `Buffer`s is 15% faster on average. (Brian White) nodejs/node#10837 - `querystring.stringify()` is up to 7.8% faster on average. (Brian White) nodejs/node#10852 - `querystring.parse()` is up to 21% faster on average. (Brian White) nodejs/node#10874 * IPC: - Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) nodejs/node#10677 - Performance gains may be up to 40% for some workloads. * child_process: - `spawnSync` now returns a null `status` when child is terminated by a signal. (cjihrig) nodejs/node#11288 - This fixes the behavior to act like `spawn()` does. * http: - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) nodejs/node#8923 - Debug messages have been added for cases when headers contain invalid values. (Evan Lucas) nodejs/node#9195 * node: - Heap statistics now support values larger than 4GB. (Ben Noordhuis) nodejs/node#10186 * timers: - Timer callbacks now always maintain order when interacting with domain error handling. (John Barboza) nodejs/node#10522 PR-URL: nodejs/node#11759 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

nodejs-github-bot added timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. lts-watch-v6.x labels Dec 29, 2016

jBarz force-pushed the domain_timers branch from 73b1d18 to 3e36d54 Compare December 29, 2016 16:59

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

gibfahn reviewed Dec 29, 2016

View reviewed changes

sam-github suggested changes Dec 29, 2016

View reviewed changes

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

jasnell approved these changes Dec 29, 2016

View reviewed changes

gibfahn approved these changes Dec 30, 2016

View reviewed changes

gibfahn requested a review from Fishrock123 December 30, 2016 10:03

sam-github suggested changes Dec 30, 2016

View reviewed changes

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

sam-github approved these changes Dec 30, 2016

View reviewed changes

Fishrock123 self-assigned this Jan 13, 2017

Fishrock123 approved these changes Jan 13, 2017

View reviewed changes

nodejs-github-bot added the lts-watch-v6.x label Jan 14, 2017

Fishrock123 suggested changes Feb 10, 2017

View reviewed changes

jBarz force-pushed the domain_timers branch from cbbaab3 to 595d9f1 Compare February 12, 2017 16:20

nodejs-github-bot removed the lts-watch-v6.x label Feb 12, 2017

jBarz force-pushed the domain_timers branch 2 times, most recently from ab06bf2 to 60c458b Compare February 12, 2017 16:26

timer,domain: maintain order of timer callbacks

a4ed667

jBarz force-pushed the domain_timers branch from 60c458b to a4ed667 Compare February 13, 2017 20:01

Fishrock123 approved these changes Feb 13, 2017

View reviewed changes

Fishrock123 closed this Feb 15, 2017

jBarz deleted the domain_timers branch February 16, 2017 14:25

italoacasas mentioned this pull request Feb 25, 2017

7.7.0 Proposal #11553

Merged

jasnell added the dont-land-on-v4.x label Mar 7, 2017

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

timer,domain: maintain order of timers upon uncaught exception #10522

timer,domain: maintain order of timers upon uncaught exception #10522

jBarz commented Dec 29, 2016 •

edited

Loading

jasnell commented Dec 29, 2016

gibfahn Dec 29, 2016 •

edited

Loading

jBarz Dec 29, 2016

sam-github Dec 29, 2016

jBarz Dec 29, 2016

sam-github Dec 29, 2016

jBarz Dec 29, 2016

gibfahn left a comment

gibfahn commented Dec 30, 2016 •

edited

Loading

sam-github Dec 30, 2016

sam-github Dec 30, 2016

jBarz Dec 30, 2016

sam-github Dec 30, 2016

jasnell commented Jan 6, 2017

Fishrock123 left a comment

Fishrock123 Jan 13, 2017

gibfahn commented Jan 19, 2017

Fishrock123 left a comment

Fishrock123 Feb 10, 2017 •

edited

Loading

jBarz commented Feb 13, 2017

Fishrock123 left a comment

Fishrock123 commented Feb 14, 2017

Fishrock123 commented Feb 15, 2017

jasnell commented Mar 7, 2017

		@@ -0,0 +1,14 @@
		const domain = require('domain').create();
		var first = false;

timer,domain: maintain order of timers upon uncaught exception #10522

timer,domain: maintain order of timers upon uncaught exception #10522

Conversation

jBarz commented Dec 29, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

jasnell commented Dec 29, 2016

gibfahn Dec 29, 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

gibfahn left a comment

Choose a reason for hiding this comment

gibfahn commented Dec 30, 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

jasnell commented Jan 6, 2017

Fishrock123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn commented Jan 19, 2017

Fishrock123 left a comment

Choose a reason for hiding this comment

Fishrock123 Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

jBarz commented Feb 13, 2017

Fishrock123 left a comment

Choose a reason for hiding this comment

Fishrock123 commented Feb 14, 2017

Fishrock123 commented Feb 15, 2017

jasnell commented Mar 7, 2017

jBarz commented Dec 29, 2016 •

edited

Loading

gibfahn Dec 29, 2016 •

edited

Loading

gibfahn commented Dec 30, 2016 •

edited

Loading

Fishrock123 Feb 10, 2017 •

edited

Loading