edited

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.

This comment has been minimized.

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?

This comment has been minimized.

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.

This comment has been minimized.

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.

This comment has been minimized.

edited

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.

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)
* `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.[#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.[#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.[#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.[#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.[#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.[#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)

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.