timers: fix clearInterval to work with timers from setTimeout #19952

remusao · 2018-04-11T16:49:49Z

According to HTML Living Standard, "either method [clearInterval or
clearTimeout] can be used to clear timers created by setTimeout() or
setInterval()."[1].

The current implementation of clearTimeout is already able to destroy a
timer created by setInterval, but not the other way around.

Refs: https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#dom-setinterval

Checklist

make -j4 jstest (UNIX)
tests are included
commit message follows commit guidelines
documentation is changed or added

Affected core subsystem(s)

timers.

apapirovski

SGTM, just a few small comments.

apapirovski · 2018-04-11T16:52:56Z

test/parallel/test-timers-clear-timeout-interval-equivalent.js

+// * Refs: https://nodejs.org/api/timers.html#timers_class_timeout
+
+const assert = require('assert');
+const Timer = require('timers');


We don't import timers in our timer tests. You can just use the global versions.

apapirovski · 2018-04-11T16:53:37Z

test/parallel/test-timers-clear-timeout-interval-equivalent.js

+
+// Disarm timeout with setInterval.
+const timeout = Timer.setTimeout(() => {
+  nbTimersTriggered += 1;


You can use common.mustNotCall instead for both of these.

apapirovski · 2018-04-11T16:53:56Z

test/parallel/test-timers-clear-timeout-interval-equivalent.js

+Timer.clearInterval(timeout);
+
+// Check that no callback was fired.
+setTimeout(() => {


Once you convert to use common.mustNotCall you can just remove this whole block.

apapirovski · 2018-04-11T16:56:47Z

test/parallel/test-timers-clear-timeout-interval-equivalent.js

+// Disarm timeout with setInterval.
+const timeout = Timer.setTimeout(() => {
+  nbTimersTriggered += 1;
+}, common.platformTimeout(20));


common.platformTimeout isn't necessary. You can also just use 1 for the timeout value since it shouldn't execute regardless.

apapirovski · 2018-04-11T16:59:49Z

lib/timers.js

+    // It's allowed to use clearInterval to kill a setTimeout, which has its
+    // _repeat attribute to `null` already.
+    if (timer._repeat) {
+      timer._repeat = null;


In all honesty, I would almost just go with exports.clearInterval = clearTimeout; The distinction is pretty meaningless especially since they're now interchangeable. Unsetting _repeat doesn't do anything. Or we could do:

exports.clearInterval = function clearInterval(timer) { clearTimeout(timer); };

Mostly to appease stack traces, etc. with differing function names.

remusao · 2018-04-11T17:07:29Z

@apapirovski Thank you so much for your review. Is it alright if I amend my commit or should I create another one to implement your suggestions?

mscdex · 2018-04-11T17:44:33Z

lib/timers.js

+  // clearTimeout and clearInterval can be used to clear timers created from
+  // both setTimeout and setInterval, as specified by HTML Living Standard:
+  // https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#dom-setinterval
+  clearTimeout(timer);


Can't we just set it to the same function instead of creating this wrapper?

FWIW I brought up this option for the sake of users who care about the function name for profilers, stack traces and the like. I personally would just go with exports.clearInterval = clearTimeout; but I'm not certain how other collaborators might feel about it.

apapirovski

A few small nits left. (And yep, amending commits is perfectly fine.)

apapirovski · 2018-04-11T18:16:00Z

test/parallel/test-timers-clear-timeout-interval-equivalent.js

+// * Refs: https://nodejs.org/api/timers.html#timers_class_timeout
+
+// Disarm interval with clearTimeout.
+const interval = setInterval(common.mustNotCall(), common.platformTimeout(20));


Nit: common.platformTimeout(20) can just become 1 now. We only use common.platformTimeout when the test is timing sensitive. Since in this test no timeouts will execute — unless it's broken — it doesn't need that extra overhead.

apapirovski · 2018-04-11T18:16:09Z

test/parallel/test-timers-clear-timeout-interval-equivalent.js

+const interval = setInterval(common.mustNotCall(), common.platformTimeout(20));
+clearTimeout(interval);
+
+// Disarm timeout with setInterval.


s/setInterval/clearInterval

BridgeAR

LGTM with @apapirovski comments addressed.

Fishrock123

Could you please set timer._repeat = null; in clearTimeout? Thanks!

Also, here's a CI run: https://ci.nodejs.org/job/node-test-pull-request/14210/

Fishrock123 · 2018-04-11T22:19:48Z

test/parallel/test-timers-clear-timeout-interval-equivalent.js

+// clearInterval and that timers created with setInterval can be disarmed by
+// clearTimeout.
+//
+// This behavior is documented in Node.js' documentation and the HTML Living


Also, I don't see this behavior noted in the Node.js docs? This PR also doesn't contain doc updates.

It can be passed to clearTimeout() or clearInterval() (respectively) in order to cancel the scheduled actions.

Is not the same as "it can be passed to either". I'm impartial to documenting it, but mentions of the docs should be correct. :)

You are right, I misunderstood the current documentation. Will update it. Thanks!

apapirovski · 2018-04-12T06:13:54Z

Could you please set timer._repeat = null; in clearTimeout? Thanks!

@Fishrock123 is there a specific reason we want this?

To elaborate: _repeat used to be a function reference (which then held a reference to the interval, so it could've potentially leaked memory) so any clearing that used to happen was to remove that reference. After we made _repeat just be null or the same value as _idleTimeout, I see no reason that clearing it is necessary.

remusao · 2018-04-12T07:13:46Z

Thank you for the reviews. I updated with the following changes:

Amend commit to be accurate regarding the state of the documentation.
Fix documentation to reflect the new behavior.
Simplify tests.

According to HTML Living Standard, "either method [clearInterval or clearTimeout] can be used to clear timers created by setTimeout() or setInterval()."[1]. The current implementation of clearTimeout is already able to destroy a timer created by setInterval, but not the other way around. Refs: https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#dom-setinterval

Fishrock123

Ok seems good to me. I was thinking it was still a function.

I suppose if it was set and the timer somehow got re-enrolled we'd want to keep it anyways (thinking of when we might support extending the timers prototype).

remusao · 2018-04-13T08:40:52Z

@Fishrock123 Thanks for your review. Let me know if there is anything I can do on this one.

BridgeAR · 2018-04-15T21:42:45Z

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

apapirovski · 2018-04-15T22:25:09Z

FWIW I do think this could potentially be semver-major since it would make clearInterval clear timeouts which it didn't used to. Thoughts?

BridgeAR · 2018-04-15T22:43:53Z

@apapirovski well in that case the user originally intended to clear the timeout and it was a bug in the users code that it was not cleared. So it seems like a bug fix to me. We can of course run a CITGM to see if anything pops up.

apapirovski · 2018-04-15T22:49:53Z

well in that case the user originally intended to clear the timeout and it was a bug in the users code that it was not cleared.

Or they have some weird structure that stores both timeouts and intervals but only want to clear the intervals. And yes, I know it's far fetched but just pointing it out.

We should at least let this bake before back-porting to LTS.

According to HTML Living Standard, "either method [clearInterval or clearTimeout] can be used to clear timers created by setTimeout() or setInterval().". The current implementation of clearTimeout is already able to destroy a timer created by setInterval, but not the other way around. PR-URL: nodejs#19952 Refs: https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#dom-setinterval Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

BridgeAR · 2018-04-16T02:44:19Z

Landed in 978e152 🎉

@remusao congratulations on your first commit to Node.js and to officially becoming a contributor! 👍

remusao · 2018-04-16T07:51:24Z

@BridgeAR Thank you very much! That was a very interesting experience and I will do it again if I can. I have to say that the documentation as to how to contribute was very detailed.

According to HTML Living Standard, "either method [clearInterval or clearTimeout] can be used to clear timers created by setTimeout() or setInterval().". The current implementation of clearTimeout is already able to destroy a timer created by setInterval, but not the other way around. PR-URL: #19952 Refs: https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#dom-setinterval Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

MylesBorins · 2018-08-17T19:18:41Z

Is this something we want to backport to v8.x?

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

apapirovski approved these changes Apr 11, 2018

View reviewed changes

remusao force-pushed the fix-clear-interval branch from 4ac68a0 to 9f7630d Compare April 11, 2018 17:24

mscdex reviewed Apr 11, 2018

View reviewed changes

apapirovski approved these changes Apr 11, 2018

View reviewed changes

BridgeAR approved these changes Apr 11, 2018

View reviewed changes

Fishrock123 suggested changes Apr 11, 2018

View reviewed changes

Fishrock123 reviewed Apr 11, 2018

View reviewed changes

remusao force-pushed the fix-clear-interval branch from 9f7630d to fd5a7e9 Compare April 12, 2018 07:14

remusao force-pushed the fix-clear-interval branch from fd5a7e9 to 780176e Compare April 12, 2018 07:15

Fishrock123 approved these changes Apr 12, 2018

View reviewed changes

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

TimothyGu approved these changes Apr 15, 2018

View reviewed changes

BridgeAR closed this Apr 16, 2018

BridgeAR added the baking-for-lts PRs that need to wait before landing in a LTS release. label Apr 16, 2018

targos removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Sep 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

timers: fix clearInterval to work with timers from setTimeout #19952

timers: fix clearInterval to work with timers from setTimeout #19952

remusao commented Apr 11, 2018 •

edited

Loading

apapirovski left a comment

apapirovski Apr 11, 2018

apapirovski Apr 11, 2018

apapirovski Apr 11, 2018

apapirovski Apr 11, 2018

apapirovski Apr 11, 2018

remusao commented Apr 11, 2018 •

edited

Loading

mscdex Apr 11, 2018

apapirovski Apr 11, 2018

apapirovski left a comment •

edited

Loading

apapirovski Apr 11, 2018

apapirovski Apr 11, 2018

BridgeAR left a comment

Fishrock123 left a comment •

edited

Loading

Fishrock123 Apr 11, 2018

Fishrock123 Apr 11, 2018

remusao Apr 12, 2018

apapirovski commented Apr 12, 2018 •

edited

Loading

remusao commented Apr 12, 2018

Fishrock123 left a comment

remusao commented Apr 13, 2018

BridgeAR commented Apr 15, 2018

apapirovski commented Apr 15, 2018

BridgeAR commented Apr 15, 2018

apapirovski commented Apr 15, 2018

BridgeAR commented Apr 16, 2018

remusao commented Apr 16, 2018

MylesBorins commented Aug 17, 2018

timers: fix clearInterval to work with timers from setTimeout #19952

timers: fix clearInterval to work with timers from setTimeout #19952

Conversation

remusao commented Apr 11, 2018 • edited Loading

Checklist

Affected core subsystem(s)

apapirovski 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

remusao commented Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

Fishrock123 left a comment • 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

apapirovski commented Apr 12, 2018 • edited Loading

remusao commented Apr 12, 2018

Fishrock123 left a comment

Choose a reason for hiding this comment

remusao commented Apr 13, 2018

BridgeAR commented Apr 15, 2018

apapirovski commented Apr 15, 2018

BridgeAR commented Apr 15, 2018

apapirovski commented Apr 15, 2018

BridgeAR commented Apr 16, 2018

remusao commented Apr 16, 2018

MylesBorins commented Aug 17, 2018

remusao commented Apr 11, 2018 •

edited

Loading

remusao commented Apr 11, 2018 •

edited

Loading

apapirovski left a comment •

edited

Loading

Fishrock123 left a comment •

edited

Loading

apapirovski commented Apr 12, 2018 •

edited

Loading