Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions doc/api/timers.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ Returns a reference to the `Immediate`.
## Class: Timeout

This object is created internally and is returned from [`setTimeout()`][] and
[`setInterval()`][]. It can be passed to [`clearTimeout()`][] or
[`clearInterval()`][] (respectively) in order to cancel the scheduled actions.
[`setInterval()`][]. It can be passed to either [`clearTimeout()`][] or
[`clearInterval()`][] in order to cancel the scheduled actions.

By default, when a timer is scheduled using either [`setTimeout()`][] or
[`setInterval()`][], the Node.js event loop will continue running as long as the
Expand Down
11 changes: 5 additions & 6 deletions lib/timers.js
Original file line number Diff line number Diff line change
Expand Up @@ -496,14 +496,13 @@ exports.setInterval = function(callback, repeat, arg1, arg2, arg3) {
return timeout;
};

exports.clearInterval = function(timer) {
if (timer && timer._repeat) {
timer._repeat = null;
clearTimeout(timer);
}
exports.clearInterval = function clearInterval(timer) {
// 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};


function unrefdHandle(timer, now) {
try {
// Don't attempt to call the callback if it is not a function.
Expand Down
18 changes: 18 additions & 0 deletions test/parallel/test-timers-clear-timeout-interval-equivalent.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
'use strict';
const common = require('../common');

// This test makes sure that timers created with setTimeout can be disarmed by
// clearInterval and that timers created with setInterval can be disarmed by
// clearTimeout.
//
// This behavior is documented in the HTML Living Standard:
//
// * Refs: https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#dom-setinterval

// Disarm interval with clearTimeout.
const interval = setInterval(common.mustNotCall(), 1);
clearTimeout(interval);

// Disarm timeout with clearInterval.
const timeout = setTimeout(common.mustNotCall(), 1);
clearInterval(timeout);