fix double patched timers in older node versions

[manuelschneider]

Differences between the node versions with regards to patching timers are:

• In node v0.10 the global timers are methods, which are - as @JiaLiPassion stated in fix: don't save handleIds for node #736 - lazy loading the timers-module and executing the appropriete function.
• In node v7.8.0 these are just references: global.clearImmediate = timers.clearImmediate;
• I found no other variant doing a simple git blame on the lines in nodejs.

This means:

• For v7.8.0 both entry points must be patched, because the assignment always happens before patching.
• For v0.10 only timers should be patched, to avoid 'double-wrap'

Because the tests aren't compatible to v0.10, I have no testcase for this. But getting exact stacktraces is simple:

$ node -v
v7.8.0
$ node -e 'require("./dist/zone-node.js");var a = function (){ throw new Error("test");};setImmediate(a);'
dist/zone-node.js:169
                        throw error;
                        ^

Error: test
    at Immediate.a ([eval]:1:59) [<root>]
    at Zone.runTask (dist/zone-node.js:165:47) [<root> => <root>]
    at Immediate.ZoneTask.invoke (dist/zone-node.js:460:38) [<root>]
    at Immediate.timer (dist/zone-node.js:1643:29) [<root>]
    at runCallback (timers.js:672:20) [<root>]
    at tryOnImmediate (timers.js:645:5) [<root>]
    at processImmediate [as _immediateCallback] (timers.js:617:5) [<root>]

$ node -v
v0.10.29
$ node -e 'require("./dist/zone-node.js");var a = function (){ throw new Error("test");};setImmediate(a);'

dist/zone-node.js:169
                        throw error;
                              ^
Error: test
    at new Error (<anonymous>)
    at Object.a ([eval]:1:59) [<root>]
    at Zone.runTask (dist/zone-node.js:165:47) [<root> => <root>]
    at Object.ZoneTask.invoke (dist/zone-node.js:460:38) [<root>]
    at Object.timer (dist/zone-node.js:1643:29) [<root>]
    at Zone.runTask (dist/zone-node.js:165:47) [<root> => <root>]
    at Object.ZoneTask.invoke (dist/zone-node.js:460:38) [<root>]
    at Object.timer [as _onImmediate] (dist/zone-node.js:1643:29) [<root>]
    at processImmediate [as _immediateCallback] (timers.js:336:15) [<root>]

Problem is: shouldPatchGlobalTimers is always true, because the method in old node, which does the lazy loading is not equal to the patched method from timers.

If this was the reason behind shouldPatchGlobalTimers, the linked commit should do the trick:

• Before timers are patched, timers and globals are equal in newer node versions
• Because the tests don't run in older node anyway, no testcase for that
• Existing tests ensure current node patches both.

At least the stacktraces look great after the change:

$ node -v
v7.8.0
$ node -e 'require("./dist/zone-node.js");var a = function (){ throw new Error("test");};setImmediate(a);'
dist/zone-node.js:169
                        throw error;
                        ^

Error: test
    at Immediate.a ([eval]:1:59) [<root>]
    at Zone.runTask (dist/zone-node.js:165:47) [<root> => <root>]
    at Immediate.ZoneTask.invoke (dist/zone-node.js:460:38) [<root>]
    at Immediate.timer (dist/zone-node.js:1642:29) [<root>]
    at runCallback (timers.js:672:20) [<root>]
    at tryOnImmediate (timers.js:645:5) [<root>]
    at processImmediate [as _immediateCallback] (timers.js:617:5) [<root>]

$ node -v
v0.10.29
$ node -e 'require("./dist/zone-node.js");var a = function (){ throw new Error("test");};setImmediate(a);'

dist/zone-node.js:169
                        throw error;
                              ^
Error: test
    at new Error (<anonymous>)
    at Object.a ([eval]:1:59) [<root>]
    at Zone.runTask (dist/zone-node.js:165:47) [<root> => <root>]
    at Object.ZoneTask.invoke (dist/zone-node.js:460:38) [<root>]
    at Object.timer [as _onImmediate] (dist/zone-node.js:1642:29) [<root>]
    at processImmediate [as _immediateCallback] (timers.js:336:15) [<root>]

[JiaLiPassion]

@manuelschneider , I think the purpose of the original logic is

1. if global.setTimeout don't use timer.js, both setTimeout in timer.js and global.setTimeout should be patched
2. and the original logic just compare the reference.
3. and in nodejs v0.10, global.setTimeout in fact use timer.js but the reference not equal.

So I think we should still need to patch global.setTimeout if global.setTimeout not using timer.js, to confirm that, I think we should not compare the reference, but we have to check the result which returned from setTimeout.

something like this

const timer = require('timer');
const originSetTimeout = timer.setTimeout;
let globalUseTimeoutFromTimer = false;
timer.setTimeout = function() {
  globalUseTimeoutFromTimer = true;
  return originSetTimeout.apply(this, arguments);
}
const detectTimeout = global.setTimeout(noop, 100);
clearTimeout(detectTimeout);
timer.setTimeout = originSetTimeout;
patchTimer(timer);

if (!globalUseTimeoutFromTimer) {
 patchTimer(global);
} 

And we should add test cases to simulate that global don't use timers' setTimeout/immediate/Interval case.

[mhevery]

@JiaLiPassion what do you think we should do with this PR?

[JiaLiPassion]

@misko
I think this one is not correct, and it related to other issue #437, so I will make another PR to fix them both.

And the other one #736 is ok for merge because if timers return a non-integer id, we should not keep it in tasksByHandleId.

[JiaLiPassion]

We should check whether global setTimeout using timers setTimeout

[manuelschneider]

@JiaLiPassion I don't think we should rely on the zone-patched return object to decide if the method needs patching, as it seems undocumented to me, and therefore may change in the future. This is why I went with the above solution. My quick research revealed that the mentioned ones are the only variants in 'vanilla' node.

If something else patches the timers, the assumption behind the zone-patched return objects also turns out
to be problematic: We know that node does not provide anything with the object to be done between getting
it from ie setTimeout and giving it to clearTimeout. Other stuff could do anything. Therefore I'm not sure if it
should be a requirement to support anything else than vanilla node. At least as long as zones returns a task,
instead of nodes object. The hack for getting the id via toString probably won't work here.

Only supporting vanilla node leaves us in my view with two options:

• Do something like instanceof Task and fail when the (undocumented?) zone-specific return value from
  timers changed.
• Use the above solution and fail when node changes its code for the global timers to have the global timers
  do something else than using timers.

I'd still stick to the latter, but maybe I'm not seeing a third solution here.

[manuelschneider]

Shouldn't #440 have already fixed #437? Using another entrypoint would solve the incompatibility with node here.

[JiaLiPassion]

@manuelschneider
in this PR, you check global.setTimeout===timer.setTimeout
in nodejs 0.10.x, the result is still false

var timer = require('timers');
console.log('global.setTimeout', global.setTimeout.toString());
console.log('timer.setTimeout', timer.setTimeout.toString());
console.log('equal', global.setTimeout === timer.setTimeout);

So my previous comment means we should check global.setTimeout use timers.setTimeout or not , not only check ===.

And you are right #440 fix #437, thank you for the info. But it seems the fix have been reverted.

[manuelschneider]

in this PR, you check global.setTimeout===timer.setTimeout
in nodejs 0.10.x, the result is still false

That is correct. If the reference is equal it means we have to wrap both entrypoints. If it's not equal, there's no way to know if the two functions call each other. We can assume it by either

• Comparing the return values (which is not safe in my opinion, see above)
• Looking at nodes code (which still is not safe, but a failure is (again imho) less likely and changes in future
  node versions should be detected by the existing tests as using the global setTimeout won't work any longer)

[JiaLiPassion]

@manuelschneider

That's why I think we should use detect code like this

const timer = require('timer');
const originSetTimeout = timer.setTimeout;
let globalUseTimeoutFromTimer = false;
timer.setTimeout = function() {
  globalUseTimeoutFromTimer = true;
  return originSetTimeout.apply(this, arguments);
}
const detectTimeout = global.setTimeout(noop, 100);
clearTimeout(detectTimeout);
timer.setTimeout = originSetTimeout;
patchTimer(timer);

if (!globalUseTimeoutFromTimer) {
 patchTimer(global);
} 

[manuelschneider]

That looks like a third solution :)

[JiaLiPassion]

@manuelschneider , thanks for confirm, and I would like to consider to fix this one with #437, because the #440 seems to be gone.

[MitchTalmadge]

Maybe due to this, after updating from 0.8.9 to 0.8.10, I get this error:

Uncaught Error: Already loaded patch: timers
    at Function.Zone.__load_patch (zone-mix.js:80)
    at zone-mix.js:2242
    at performance (zone-mix.js:9)
    at Object.<anonymous> (zone-mix.js:12)
    at __webpack_require__ (bootstrap ccaad62…:54)
    at Object.module.exports (polyfills.ts:22)
    at __webpack_require__ (bootstrap ccaad62…:54)
    at Object.<anonymous> (polyfills.js:20788)
    at __webpack_require__ (bootstrap ccaad62…:54)
    at bootstrap ccaad62…:151

Using Electron and require('zone.js/dist/zone-mix');

[JiaLiPassion]

@MitchTalmadge , thanks for posting the issue, I will make a PR to fix it