timers: reschedule interval even if it threw #20002

apapirovski · 2018-04-13T08:45:15Z

Node.js currently doesn't match browser behaviour when it comes to intervals that throw during their execution. In all browsers, the interval continues running but in Node.js it will never be rescheduled after a throw nor will the destroy async hook for that interval fire.

This is a semver-major change and IMO bringing us in line with the browsers is a good idea. If someone is handling uncaughtException or using domains then they should know what they're doing as far as concerns about leaking memory as a result of this.

IMO this is a pretty well documented behaviour of setInterval, at least as far as the JS ecosystem outside of Node.js is concerned.

I'm fine with getting this in v11 instead of v10 and going from there, if that's preferable.

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

Checklist

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

apapirovski · 2018-04-13T08:46:41Z

/cc @benjamingr @BridgeAR @Fishrock123 @mcollina @nodejs/timers

BridgeAR

I think I am in favor of this but I have to give the memory leak argument of @mcollina some more thoughts so I abstain from giving a vote for now.

BridgeAR · 2018-04-13T09:31:36Z

test/parallel/test-timers-interval-throw.js

+const interval = setInterval(() => { throw new Error('IntervalError'); }, 1);
+
+process.on('uncaughtException', common.mustCall((err) => {
+  assert(err.message, 'IntervalError');


This seems to miss .strictEqual.

apapirovski · 2018-04-13T09:47:43Z

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

jasnell · 2018-04-16T16:25:11Z

Ping @nodejs/tsc ... please take a look

mcollina

I don't see a compelling enough reason to change this behavior. Firstly, it has been like this from the start, and no one has complained so far (or at least, it's not linked in the issue). Secondly, it can introduce memory leaks or prevent processes from exiting while before they were working fine (the failing setInterval will keep the event loop spinning).

I'm happy to discuss and change my mind on this. What is the reason to make this change apart from increasing web compatibility? Moreover, how increasing web compatibility would benefit Node.js in this API?

Fishrock123 · 2018-04-16T18:12:25Z

Ok, thoughts:

The common case remains the same. Both bail the same. However, uncaughtException is affected.

This seems like a good thing, though. I didn't try real domains, but making an error handling abstraction using uncaughtException and Async-Hooks would require this in order to properly work with timers.

I don't see much problem with it that can't be caused already, and it does seem to have an actual use-case. This seems to fix a bug in uncaughtException & timers, from what I can tell.

That being said, that doesn't "match browser behaviour". Browser behavior does not bail on the page, it bails only on the execution 'stack', back to an interaction, I/O, or timeout. Kinda like always having a no-op uncaughtException handler.

@mcollina, any further thoughts on this?

mcollina · 2018-04-16T19:22:59Z

@Fishrock123 can you make an example of such an error handling utility? This explanation seems compelling.

Fishrock123 · 2018-04-16T19:31:20Z

Anything meant to replace domains' error handling without using domains would have to go that route. I probably don't have the time to actually build one, but it's possible that even domains itself acts this way now? I don't really remember.

mcollina

LGTM

mcollina · 2018-04-16T19:34:41Z

(not to be pulled in 10.)

apapirovski · 2018-04-16T19:35:47Z

I've added it to 11.0 milestone so that there's no confusion. Let's leave it open for a while and everyone can have the time to think about it / evaluate what it means. I see no need to rush on landing this.

BridgeAR · 2018-04-22T14:05:08Z

This needs a rebase. Otherwise this seems ready to land.

BridgeAR · 2018-04-25T11:39:15Z

@apapirovski this needs a rebase

apapirovski · 2018-04-28T16:21:27Z

CI after rebase: https://ci.nodejs.org/job/node-test-pull-request/14567/

To match browser behaviour, intervals should continue being scheduled even if the user callback threw during execution.

To match browser behaviour, intervals should continue being scheduled even if the user callback threw during execution. PR-URL: nodejs#20002 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

BridgeAR · 2018-04-29T14:51:46Z

Landed in 198eb9c 🎉

Notable changes: * Build * FreeBSD 10 is no longer supported. [#22617](#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](#20442) * `crypto` * PEM-level encryption is now supported. [#23151](#23151) * An API for key pair generation has been added. [#22660](#22660) * Dependencies * V8 has been updated to 7.0. [#22754](#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](#20270) * `http2` * An event will be emitted when a `PING` frame is received. [#23009](#23009) * Support for the `ORIGIN` frame has been added. [#22956](#22956) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. * An experimental implementation of `queueMicrotask()` has been added. [#22951](#22951) * Internal * Windows performance-counter support has been removed. [#22485](#22485) * The `--expose-http2` command-line option has been removed. [#20887](#20887) * Promises * A new `multipleResolves` event will be emitted when a Promise is resolved (or rejected) more than once. [#22218](#22218) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](#21914)

Notable changes: * Build * FreeBSD 10 is no longer supported.[#22617](#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](#20442) * Dependencies * V8 has been updated to 7.0. [#22754](#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [#22951](#22951) * Internal * Windows performance-counter support has been removed. [#22485](#22485) * The `--expose-http2` command-line option has been removed. [#20887](#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](#21914)

Notable changes: * Build * FreeBSD 10 is no longer supported.[nodejs#22617](nodejs#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [nodejs#21316](nodejs#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [nodejs#21649](nodejs#21649) * `console.time()` will no longer reset a timer if it already exists. [nodejs#20442](nodejs#20442) * Dependencies * V8 has been updated to 7.0. [nodejs#22754](nodejs#22754) * `fs` * The `fs.read()` method now requires a callback. [nodejs#22146](nodejs#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[nodejs#20735](nodejs#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [nodejs#20270](nodejs#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [nodejs#22951](nodejs#22951) * Internal * Windows performance-counter support has been removed. [nodejs#22485](nodejs#22485) * The `--expose-http2` command-line option has been removed. [nodejs#20887](nodejs#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [nodejs#20002](nodejs#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [nodejs#22281](nodejs#22281) * `util.inspect()` output size is limited to 128 MB by default. [nodejs#22756](nodejs#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [nodejs#21914](nodejs#21914)

Notable changes: * Build * FreeBSD 10 is no longer supported.[#22617](nodejs/node#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](nodejs/node#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](nodejs/node#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](nodejs/node#20442) * Dependencies * V8 has been updated to 7.0. [#22754](nodejs/node#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](nodejs/node#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](nodejs/node#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](nodejs/node#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [#22951](nodejs/node#22951) * Internal * Windows performance-counter support has been removed. [#22485](nodejs/node#22485) * The `--expose-http2` command-line option has been removed. [#20887](nodejs/node#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](nodejs/node#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](nodejs/node#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](nodejs/node#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](nodejs/node#21914)

apapirovski added timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. semver-major PRs that contain breaking changes and should be released in the next major version. labels Apr 13, 2018

nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Apr 13, 2018

apapirovski mentioned this pull request Apr 13, 2018

timers: fix subsequent enroll calls not working #19936

Closed

3 tasks

benjamingr approved these changes Apr 13, 2018

View reviewed changes

BridgeAR reviewed Apr 13, 2018

View reviewed changes

apapirovski force-pushed the patch-timers-interval-errors branch from 53ad126 to f7c5dba Compare April 13, 2018 09:46

BridgeAR requested review from mcollina, Fishrock123 and a team April 15, 2018 21:12

mcollina requested changes Apr 16, 2018

View reviewed changes

devsnek approved these changes Apr 16, 2018

View reviewed changes

mcollina approved these changes Apr 16, 2018

View reviewed changes

apapirovski added this to the 11.0.0 milestone Apr 16, 2018

jasnell added the dont-land-on-v10.x label Apr 16, 2018

Fishrock123 approved these changes Apr 17, 2018

View reviewed changes

BridgeAR approved these changes Apr 22, 2018

View reviewed changes

jasnell approved these changes Apr 23, 2018

View reviewed changes

apapirovski force-pushed the patch-timers-interval-errors branch from f7c5dba to f10c657 Compare April 28, 2018 16:21

apapirovski added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 28, 2018

timers: reschedule interval even if it threw

350b126

To match browser behaviour, intervals should continue being scheduled even if the user callback threw during execution.

apapirovski force-pushed the patch-timers-interval-errors branch from f10c657 to 350b126 Compare April 28, 2018 16:44

apapirovski added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 28, 2018

BridgeAR closed this Apr 29, 2018

apapirovski deleted the patch-timers-interval-errors branch April 30, 2018 06:26

targos removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dont-land-on-v10.x labels Jun 8, 2018

jasnell mentioned this pull request Oct 2, 2018

11.0.0 proposal #23230

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

timers: reschedule interval even if it threw #20002

timers: reschedule interval even if it threw #20002

apapirovski commented Apr 13, 2018 •

edited

Loading

apapirovski commented Apr 13, 2018

BridgeAR left a comment

BridgeAR Apr 13, 2018

apapirovski commented Apr 13, 2018

jasnell commented Apr 16, 2018

mcollina left a comment

Fishrock123 commented Apr 16, 2018

mcollina commented Apr 16, 2018

Fishrock123 commented Apr 16, 2018

mcollina left a comment

mcollina commented Apr 16, 2018

apapirovski commented Apr 16, 2018 •

edited

Loading

BridgeAR commented Apr 22, 2018

BridgeAR commented Apr 25, 2018

apapirovski commented Apr 28, 2018 •

edited

Loading

BridgeAR commented Apr 29, 2018

timers: reschedule interval even if it threw #20002

timers: reschedule interval even if it threw #20002

Conversation

apapirovski commented Apr 13, 2018 • edited Loading

Checklist

apapirovski commented Apr 13, 2018

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR Apr 13, 2018

Choose a reason for hiding this comment

apapirovski commented Apr 13, 2018

jasnell commented Apr 16, 2018

mcollina left a comment

Choose a reason for hiding this comment

Fishrock123 commented Apr 16, 2018

mcollina commented Apr 16, 2018

Fishrock123 commented Apr 16, 2018

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Apr 16, 2018

apapirovski commented Apr 16, 2018 • edited Loading

BridgeAR commented Apr 22, 2018

BridgeAR commented Apr 25, 2018

apapirovski commented Apr 28, 2018 • edited Loading

BridgeAR commented Apr 29, 2018

apapirovski commented Apr 13, 2018 •

edited

Loading

apapirovski commented Apr 16, 2018 •

edited

Loading

apapirovski commented Apr 28, 2018 •

edited

Loading