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.

Sign up for GitHub

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

Jump to bottom

timers: fixing API refs to use safe internal refs #5882

Closed
wants to merge 2 commits into from
Closed

timers: fixing API refs to use safe internal refs #5882

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f5e17b
timers: fixing API refs to use safe internal refs
getify Aug 22, 2015
e1f5bb8
test: confirm globals not used internally
Trott Mar 24, 2016
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
16 changes: 8 additions & 8 deletions lib/timers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ const unrefedLists = {};

// Schedule or re-schedule a timer.
// The item must have been enroll()'d first.
exports.active = function(item) {
const active = exports.active = function(item) {
insert(item, false);
};

Expand Down Expand Up @@ -351,19 +351,19 @@ exports.setTimeout = function(callback, after) {

if (process.domain) timer.domain = process.domain;

exports.active(timer);
active(timer);

return timer;
};


exports.clearTimeout = function(timer) {
const clearTimeout = exports.clearTimeout = function(timer) {
if (timer && (timer[kOnTimeout] || timer._onTimeout)) {
timer[kOnTimeout] = timer._onTimeout = null;
if (timer instanceof Timeout) {
timer.close(); // for after === 0
} else {
exports.unenroll(timer);
unenroll(timer);
}
}
};
Expand Down Expand Up @@ -409,7 +409,7 @@ exports.setInterval = function(callback, repeat) {
timer._repeat = ontimeout;

if (process.domain) timer.domain = process.domain;
exports.active(timer);
active(timer);

return timer;

Expand All @@ -425,7 +425,7 @@ exports.setInterval = function(callback, repeat) {
this._handle.start(repeat, 0);
} else {
timer._idleTimeout = repeat;
exports.active(timer);
active(timer);
}
}
};
Expand Down Expand Up @@ -468,7 +468,7 @@ Timeout.prototype.unref = function() {

// Prevent running cb again when unref() is called during the same cb
if (this._called && !this._repeat) {
exports.unenroll(this);
unenroll(this);
return;
}

Expand Down Expand Up @@ -496,7 +496,7 @@ Timeout.prototype.close = function() {
this._handle[kOnTimeout] = null;
this._handle.close();
} else {
exports.unenroll(this);
unenroll(this);
}
return this;
};
Expand Down
21 changes: 21 additions & 0 deletions test/parallel/test-timers-api-refs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
'use strict';
const common = require('../common');
const timers = require('timers');

// delete global APIs to make sure they're not relied on by the internal timers
// code
delete global.setTimeout;
delete global.clearTimeout;
delete global.setInterval;
delete global.clearInterval;
delete global.setImmediate;
delete global.clearImmediate;
Copy link
Contributor

Choose a reason for hiding this comment

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

You've also changed active, but I'm not comfortable enforcing that in the tests. We should have a policy if module exports are always overridable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that if you override timers.active(), then you're on your own and have to manage any possible side effects yourself. (That said, I do think it's worthwhile for timers.js to guard against the problem internally, as it does in this PR, as long as there's no reason to suspect a performance hit or other unanticipated side effect.)


const timeoutCallback = () => { timers.clearTimeout(timeout); };
const timeout = timers.setTimeout(common.mustCall(timeoutCallback), 1);

const intervalCallback = () => { timers.clearInterval(interval); };
const interval = timers.setInterval(common.mustCall(intervalCallback), 1);

const immediateCallback = () => { timers.clearImmediate(immediate); };
const immediate = timers.setImmediate(immediateCallback);