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

Trott · 2016-03-23T23:39:20Z

Pull Request check-list

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to [CONTRIBUTING.md][0]?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Affected core subsystem(s)

timers

Description of change

Added safe internal references for 'clearTimeout(..)', 'active(..)', and
'unenroll(..)'. Changed various API refs from 'export.*' to use these
safe internal references.

Now, overwriting the global API identifiers does not create potential
breakage and/or race conditions. See Issue #2493.

Refs: #2493
Refs: #2500

/cc @getify @Fishrock123 @targos

Fishrock123 · 2016-03-24T00:03:16Z

lib/timers.js

+  var msecs = item._idleTimeout;
+  if (msecs >= 0)
+    insert(item, msecs);
+};


exports.active() is defined at the top of the file

Whoops, fixed.

Fishrock123 · 2016-03-24T00:03:53Z

pretty sure I thought this was a bad idea for some reason

Edit: ¯\_(ツ)_/¯

Fishrock123 · 2016-03-24T00:06:27Z

test/parallel/test-timers-api-refs.js

+
+// try overriding global APIs to make sure
+// they're not relied on by the timers
+global.clearTimeout = assert.fail;


needs to test all 6 global timers functions

You can't access 3 of the six. They're on the public API of the module, but that's not available to user code like tests.

I don't think I understand?

if any of:

setTimeout() clearTimeout() setInterval() clearInterval() setImmediate() clearImmediate()

Are using the exported or global version for specific internal-only behavior and then someone overwrites them, there will be problems...?

(Not much use in testing active though. Overwriting exported methods on core modules is no way guaranteed to be be safe.)

sorry i was recalling that we couldn't test the active and enroll methods, etc. I also didn't know the *immediate stuff was in here. perhaps that was put in this file sometime after my initial PR when I looked at this code last.

@Fishrock123 I've updated the test to something that is what you describe, as I understand it. PTAL

Added safe internal references for 'clearTimeout(..)', 'active(..)', and 'unenroll(..)'. Changed various API refs from 'export.*' to use these safe internal references. Now, overwriting the global API identifiers does not create potential breakage and/or race conditions. See Issue nodejs#2493.

getify · 2016-03-24T22:22:11Z

test/parallel/test-timers-api-refs.js

@@ -1,20 +1,24 @@
 'use strict';
 const common = require('../common');
-const assert = require('assert');
+const timers = require('timers');


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

Already appears in five other tests!

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

Trott · 2016-03-25T04:05:28Z

CI: https://ci.nodejs.org/job/node-test-commit/2689/

Trott · 2016-03-25T05:56:48Z

Single failure in CI is an unrelated known flaky test.

jasnell · 2016-03-26T04:17:01Z

LGTM if @Fishrock123 is happy.

jasnell · 2016-03-26T04:18:02Z

Would definitely prefer to have this sit in at least one v5 release for a week or two before backporting to v4

Fishrock123 · 2016-03-28T14:52:01Z

test/parallel/test-timers-api-refs.js

+delete global.setInterval;
+delete global.clearInterval;
+delete global.setImmediate;
+delete global.clearImmediate;


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.

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

Fishrock123 · 2016-03-28T14:53:57Z

LGTM for the globals. This can land on 4.x with little issue imo. (Might need to resolve conflicts if you haven't landed my timers refactor, but it should be an easy resolution.)

The code poses zero realistic risk if the CI passes

Trott · 2016-03-28T18:15:48Z

@Fishrock123 Would you prefer this land as-is or would you prefer I back out the changes to active()?

I think the change to active() makes sense, but if you have any doubts or concerns, I'm happy to back it out and anyone that feels strongly about it can put it in a separate PR.

On the other hand, if it's OK by you, then I will land as-is.

Fishrock123 · 2016-03-28T18:18:13Z

@Trott I think it is best to not make any guarantees in that area unless we do it globally, LGTM either way.

Added safe internal references for 'clearTimeout(..)', 'active(..)', and 'unenroll(..)'. Changed various API refs from 'export.*' to use these safe internal references. Now, overwriting the global API identifiers does not create potential breakage and/or race conditions. See Issue nodejs#2493. PR-URL: nodejs#5882 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Fixes: nodejs#2493

PR-URL: nodejs#5882 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

Trott · 2016-03-28T23:19:23Z

Landed in 9fa25c8 and f0367d0

Added safe internal references for 'clearTimeout(..)', 'active(..)', and 'unenroll(..)'. Changed various API refs from 'export.*' to use these safe internal references. Now, overwriting the global API identifiers does not create potential breakage and/or race conditions. See Issue #2493. PR-URL: #5882 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Fixes: #2493

PR-URL: #5882 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

Added safe internal references for 'clearTimeout(..)', 'active(..)', and 'unenroll(..)'. Changed various API refs from 'export.*' to use these safe internal references. Now, overwriting the global API identifiers does not create potential breakage and/or race conditions. See Issue #2493. PR-URL: #5882 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Fixes: #2493

PR-URL: #5882 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

MylesBorins · 2016-04-11T15:15:02Z

@Fishrock123 this relies on your other timer changes correct? I think the idea was to let those changes live in v5 a little longer to catch edges (such as this one).

Fishrock123 · 2016-04-11T15:36:34Z

Which changes? This should land cleanly onto v5.x I think, but it does still apply (not in the git sense) to v4.x even without the other patches.

Trott added timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. lts-watch-v4.x labels Mar 23, 2016

Fishrock123 reviewed Mar 24, 2016
View reviewed changes

Trott force-pushed the fix-timers-api-refs branch 3 times, most recently from 1cdaeae to ad31a52 Compare March 24, 2016 22:18

getify reviewed Mar 24, 2016
View reviewed changes

test: confirm globals not used internally

e1f5bb8

Trott force-pushed the fix-timers-api-refs branch from ad31a52 to e1f5bb8 Compare March 24, 2016 23:56

Trott mentioned this pull request Mar 27, 2016

minor issue with timers.js #2493

Closed

Fishrock123 reviewed Mar 28, 2016
View reviewed changes

Trott added a commit to Trott/io.js that referenced this pull request Mar 28, 2016

test: confirm globals not used internally

f0367d0

PR-URL: nodejs#5882 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

Trott closed this Mar 28, 2016

evanlucas pushed a commit that referenced this pull request Mar 30, 2016

test: confirm globals not used internally

f616adb

PR-URL: #5882 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

evanlucas mentioned this pull request Mar 31, 2016

v5.10.0 Release Proposal #5970

Closed

evanlucas pushed a commit that referenced this pull request Mar 31, 2016

test: confirm globals not used internally

3238bff

PR-URL: #5882 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Aug 30, 2016

Trott deleted the fix-timers-api-refs branch January 13, 2022 22:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Trott commented Mar 23, 2016

Fishrock123 Mar 24, 2016

Trott Mar 24, 2016

Fishrock123 commented Mar 24, 2016

Fishrock123 Mar 24, 2016

getify Mar 24, 2016

Fishrock123 Mar 24, 2016

Fishrock123 Mar 24, 2016

getify Mar 24, 2016

Trott Mar 24, 2016

getify Mar 24, 2016

Trott Mar 24, 2016

Trott Mar 24, 2016

Trott commented Mar 25, 2016

Trott commented Mar 25, 2016

jasnell commented Mar 26, 2016

jasnell commented Mar 26, 2016

Fishrock123 Mar 28, 2016

Trott Mar 28, 2016

Fishrock123 commented Mar 28, 2016

Trott commented Mar 28, 2016

Fishrock123 commented Mar 28, 2016

Trott commented Mar 28, 2016

MylesBorins commented Apr 11, 2016

Fishrock123 commented Apr 11, 2016

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

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

Conversation

Trott commented Mar 23, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Mar 24, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Mar 25, 2016

Trott commented Mar 25, 2016

jasnell commented Mar 26, 2016

jasnell commented Mar 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Mar 28, 2016

Trott commented Mar 28, 2016

Fishrock123 commented Mar 28, 2016

Trott commented Mar 28, 2016

MylesBorins commented Apr 11, 2016

Fishrock123 commented Apr 11, 2016