Skip to content

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

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

Closed
Trott wants to merge 2 commits into from
Closed

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

Changes from 1 commit
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
Prev Previous commit
test: confirm globals not used internally
  • Loading branch information
@Trott
Trott committed Mar 24, 2016
commit e1f5bb814dce9d9ac7fb3178a5a79fd7ccd3930c
29 changes: 15 additions & 14 deletions test/parallel/test-timers-api-refs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,21 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const timers = require('timers');
Copy link
Copy Markdown
Contributor

@getify getify Mar 24, 2016

Choose a reason for hiding this comment

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

is this possible? IIRC from before, you couldn't actually manually load internal modules from user code like tests.

Copy link
Copy Markdown
Member Author

@Trott Trott Mar 24, 2016

Choose a reason for hiding this comment

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

Already appears in five other tests!

Copy link
Copy Markdown
Member Author

@Trott Trott Mar 24, 2016

Choose a reason for hiding this comment

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

(Also: Yeah, it works. Test fails with a ReferenceError in Node 5.9.1, as it should, and passes with your changes in lib/timers.js.)


// don't verify the globals for this test
common.globalCheck = false;
// 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
Copy Markdown
Contributor

@Fishrock123 Fishrock123 Mar 28, 2016

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
Copy Markdown
Member Author

@Trott Trott Mar 28, 2016

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.)


// try overriding global APIs to make sure
// they're not relied on by the timers
global.clearTimeout = assert.fail;
const timeoutCallback = () => { timers.clearTimeout(timeout); };
const timeout = timers.setTimeout(common.mustCall(timeoutCallback), 1);

// run timeouts/intervals through the paces
const intv = setInterval(function() {}, 1);

setTimeout(function() {
clearInterval(intv);
}, 100);

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

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