minor issue with timers.js

[getify]

This line:

clearTimeout(timer);

Instead of calling exports.clearTimeout(..), or even better, having a local identifier pointing at that function, this line is making a call that relies on resolving to the identifier on the global object.

That works OK until a lib like mine, stable-timers, comes along and replaces those global timer API methods. When using my lib, I'm getting race conditions where sometimes this line 270 throws an exception that timer._repeat is not a function.

Could we make a small tweak to timers.js to have line 290 reference a reliable internal reference for clearTimeout?

[cjihrig]

Unless someone objects, I don't see any harm in doing that. Care to make a PR?

[silverwind]

Seems to be a reasonable change. Not sure if having a local reference is actually different from just using exports.clearTimeout.

[ChALkeR]

off-topic: @getify, your stable-timers auto-replaces the global timers upon loading the module. Before you do a release, it could be a good idea to change the module so it returns an object that has your setTimeout, clearTimeout, … methods and an extra method, for example, globalize(), that registers those in the global object for someone who really needs it. Then require('stable-timers').globalize() would do the same as require('stable-timers') does now, and it would be also possible to use the module somewhere without replacing the timers everywhere.

Monkey-patching already defined methods and/or registering stuff to the global object is generally unsafe. It also makes your lib unusable by anything but the top-level program itself (it could not be used by modules).

I'm +0 on the proposed change, though — it should make things a bit safer.

[Fishrock123]

Seems to be a reasonable change. Not sure if having a local reference is actually different from just using exports.clearTimeout.

Someone could override the exports afaik. (doesn't setting things on the object that comes out of require() do that?)((note: if you do this and break stuff your warranty is void.))

Re: the patch: +0, though I don't really think overriding global timers is a great idea.

Re: stable-timers: please let us know what issues the built-in timers have, we may be able to fix these things! :)

[getify]

thanks for the feedback. will submit a PR right away, and to be safe, will use an internal reference.

Re: stable-timers: please let us know what issues the built-in timers have

This blog post On the nature of timers explains the issues of "stability" I'm exploring fixing. Specifically, there's two stability characteristics that I'm looking at. Right now, stable-timers only implements a fix for the first, which shouldn't break anything. But the second is more disruptive, so it may end up being an optional flag that controls that.

[thefourtheye]

@getify Please don't forget to include a test case as well, if possible :-) PTAL at our contribution guide.

[getify]

quick clarification:

looking at the rest of this file in question, I see a line like 119:

const unenroll = exports.unenroll = function(item) {

Seems like a good precedent for how I should fix my issue.

However, searching the rest of the file, there are places that use unenroll(..) and some use exports.unenroll(..). Should I fix those inconsistencies?

Also, exports.active(..) is called in several places... should I fix those by making a const active = as well?

Basically, should I fix this stuff for consistency, or just constrain my change to only the one thing that I think is affecting my issue?

[Fishrock123]

@getify consistency should be fine to do. :)

[getify]

OK, I have a branch commit ready, I believe: https://github.com/getify/node/commit/73116effd9034dc33bdef034ceedfe2bfbe76623

However, my local environment here doesn't have the ability to actually build node (complains of too-old compiler), and even when I tried the steps in the Contributions guide for running individual tests, they failed to run Can't find shell executable: 'out/Release/iojs'.

Should I submit a PR as-is, or can someone else try my branch's commit and see if it's OK? Apologies for not being able to do the full steps myself.

[Fishrock123]

Sure. Here's a CI run: https://jenkins-iojs.nodesource.com/job/node-test-commit/255/

Apologies for not being able to do the full steps myself.

If you could sort that out somehow though, it will make it easier to contribute/test in the future. :)
gcc / g++ is too old I assume?, that happened in a v8 bump a while ago.

they failed to run Can't find shell executable: 'out/Release/iojs'.

Yes the tests require you to build io.js. :P

[Fishrock123]

Also, could you please run make jslint? :)

[Fishrock123]

Hmm yeah that patch definitely doesn't work.

[getify]

Perhaps there are places in the code that have relied on being able to monkeypatch parts of this API, like the active(..) or enroll(..)?

[Fishrock123]

Perhaps there are places in the code that have relied on being able to monkeypatch parts of this API

Definitely not. :p

You forgot to assign a local enroll: https://github.com/getify/node/commit/73116effd9034dc33bdef034ceedfe2bfbe76623#diff-0a5d4868b2b9b17cf9e2c11f1bd1311eL136

[getify]

Oh, no... actually, I accidentally changed "exports.unenroll" to "enroll()". Gah, my bad. Very sorry.

[getify]

Amended commit: https://github.com/getify/node/commit/67b552f4f2b08b4b4040fa10a607196f099f75d2

Yes, I will try to work on getting a local build environment that can build node and jslint for future contributions.

[Fishrock123]

New CI run: https://jenkins-iojs.nodesource.com/job/node-test-commit/257/

[getify]

well clearly that's not going well. :(

rather than bothering you to keep running tests for me, I guess I should hold off until I can get a working local build env to test/debug myself.

[getify]

The test error appears to be:

not ok 676 - test-timers-api-refs.js
Unknown globals: clearTimeout,unenroll,active

Apparently I'm not fully clear on how globals get registered in the node environment. I see other timers tests that are able to reference globals like setTimeout, and I assumed that meant you could also reference them fully qualified, like global.clearTimeout. It appears that's not the case. I'll try to investigate more.

[Fishrock123]

@getify ah looks like we have a thing in our common test impl that doesn't like this: https://github.com/nodejs/node/blob/master/test/common.js#L238-L325

[getify]

Interesting. Also, I am wondering... perhaps active and unenroll are not actually ever globals, even though they are exports of the timers module? How do you access the exported object of the timers module? Seems like the test should not only test the globals being replaced but the exports being replaced.

[Fishrock123]

perhaps active and unenroll are not actually ever globals

That's correct. See:

node/src/node.js

Lines 169 to 177 in ec6e5c7

	startup.globalTimeouts = function() {
	const timers = NativeModule.require('timers');
	global.clearImmediate = timers.clearImmediate;
	global.clearInterval = timers.clearInterval;
	global.clearTimeout = timers.clearTimeout;
	global.setImmediate = timers.setImmediate;
	global.setInterval = timers.setInterval;
	global.setTimeout = timers.setTimeout;
	};

[getify]

OK, thanks. So if I fix active and unenroll, am I to understand I'll still have a problem with the overwriting of clearTimeout in the test? What I'm getting at is I'm not sure why that error message was mentioning clearTimeout as an "unknown global" unless the point of that verification code is to prevent you from replacing globals?

[Fishrock123]

I'm not sure, I think it should work. In any case: https://github.com/nodejs/node/blob/master/test/common.js#L315-L316

[getify]

In my latest update:

1. I've addressed the lint errors.
2. Taken out any testing of active and unenroll. Unless I'm missing something, I don't think the test code has access to NativeModule.require(..), so I don't think there's a way to test against the timers module exports.

[Fishrock123]

Ok the reason why it errors on clearTimeout is that is specifically checks for the index of the value 9the function), not the key. https://github.com/nodejs/node/blob/master/test/common.js#L307-L308

[getify]

specifically checks for the index of the value (the function), not the key

Yeah, that's what I suspected when I said earlier:

unless the point of that verification code is to prevent you from replacing globals?

On your suggestion, I've turned off the global check for this test. :)

[Fishrock123]

How about ya make a PR? :)

[Trott]

Proposed PR to fix this is at #5882