fix: don't save handleIds for node #736

manuelschneider · 2017-04-13T22:05:45Z

Older node versions (0.10, still default in debian stable) will throw exceptions while trying to use the objects returned nodes Timers implementation as keys -> don't make them try it.
For the same reason - as far as I understand it - this has never worked in node, also newer versions, anyway. Therefore this shouldn't break anything.

googlebot · 2017-04-13T22:05:48Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

googlebot · 2017-04-13T22:05:48Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

manuelschneider · 2017-04-13T22:43:44Z

I signed it!

googlebot · 2017-04-13T22:43:45Z

CLAs look good, thanks!

googlebot · 2017-04-13T22:43:45Z

CLAs look good, thanks!

JiaLiPassion

I think the issue is not the handleId itself, I run v0.10, it seems the returned handleId is the zone patched version not the native version, in 0.10, the setTimeout look like this.

function () {
      var t = NativeModule.require('timers');
      return t.setTimeout.apply(this, arguments);
    }

so I think we should not just check the handleId here is number or not, because it will not totally resolve the issue, it just like the error not thrown, we should find a way to let v0.10 return native version of timeout.

manuelschneider · 2017-04-14T08:57:02Z

Basically you're right, of course. Simply suppressing errors is a bad idea in general.

I came up with this idea for a node version of 'tasksByHandleId' (stripped js-version):

function patchTimer(window, setName, cancelName, nameSuffix) {
    var objectHandles = [];
    var tasksByHandleId = {};
    function scheduleTask(task) {
            finally {
                delete objectHandles[data.handleId];
                delete tasksByHandleId[data.handleId];
            }
        }
        if (objectHandles.length > 3000000) {
            var newObjectHandles = [];
            var newTasksByHandleId = {};
            for (var i in tasksByHandleId) {
                tasksByHandleId[i].data.handleId = newObjectHandles.length;
                newObjectHandles[tasksByHandleId[i].data.handleId] = objectHandles[i];
                newTasksByHandleId[tasksByHandleId[i].data.handleId] = tasksByHandleId[i];
            }
            objectHandles = newObjectHandles;
            tasksByHandleId = newTasksByHandleId;
        }
        data.handleId = objectHandles.length;
        objectHandles[data.handleId] = setNative.apply(window, data.args);
        tasksByHandleId[data.handleId] = task;
    }
    function clearTask(task) {
        var objectHandle = objectHandles[task.data.handleId];
        delete objectHandles[task.data.handleId];
        delete tasksByHandleId[task.data.handleId];
        return clearNative(objectHandle);
    }

It puts the objects in an array, uses the index as handle id, and does the laundry now and then.

I decided not to pursue it further, because apart from timeout.ref() and timeout.unref() (which are probably what is handled in lines 75 and 76 of timers.ts), the returned object is only good for being
passed to clear*, which is afaik again patched by zones. In that case it would be a waste of
ressources to have this mapping in node.

What do you think?

JiaLiPassion · 2017-04-14T10:20:30Z

@manuelschneider , I think the reason of the issue is in nodejs v0.10, nodejs lazy load timers, so global.setTimeout !== timers.setTimeout, and zone.js patch them all.

I think we just remove global patch is ok, but I think it will need some test.

in node.ts, currently the logic is

// Timers
const timers = require('timers');
patchTimer(timers, set, clear, 'Timeout');
patchTimer(timers, set, clear, 'Interval');
patchTimer(timers, set, clear, 'Immediate');

const shouldPatchGlobalTimers = global.setTimeout !== timers.setTimeout;

if (shouldPatchGlobalTimers) {
  patchTimer(_global, set, clear, 'Timeout');
  patchTimer(_global, set, clear, 'Interval');
  patchTimer(_global, set, clear, 'Immediate');
}

I think we can change it to

// Timers
const timers = require('timers');
patchTimer(timers, set, clear, 'Timeout');
patchTimer(timers, set, clear, 'Interval');
patchTimer(timers, set, clear, 'Immediate');

it will be ok, I think, but we should add some test and findout why we patch global before.

manuelschneider · 2017-04-15T10:11:50Z

When you skip the global patches, the existing tests fail at

build/test/common/setInterval.spec.js:26
                global[utils_1.zoneSymbol('setTimeout')](function () {

If you remove the timers patch instead, at least the existing tests are ok and it seem to work. However,
at the moment I don't understand why the original author put it there.

I still don't understand, why this is related to the exceptions? From what I understand, they're simply a
type problem: handleId is everywhere described and handled as integer, not object. Trying to use it as keys of an object triggers the exceptions in older node versions.

JiaLiPassion · 2017-04-15T10:17:55Z

@manuelschneider , the test code failed is reasonable, if we modify that way, we will modify the test case as well.

If you remove the timers patch instead, at least the existing tests are ok and it seem to work. However,
at the moment I don't understand why the original author put it there.

Yeah, I don't know either. I will try to find out why.

I still don't understand, why this is related to the exceptions? From what I understand, they're simply a
type problem: handleId is everywhere described and handled as integer, not object. Trying to use it as keys of an object triggers the exceptions in older node versions.

I believe that in nodejs v0.10, the timer.setTimeout still return an integer, not object, the reason it returns object because it return a ZoneTask(which is patched by zone.js), so it will cause the error.

So I think the real solution should let setTimeout still return integer correctly.

We need to add some tests to simulate the nodejs v0.10 behavior.

manuelschneider · 2017-04-15T10:23:27Z

I believe that in nodejs v0.10, the timer.setTimeout still return an integer, not object, the reason it
returns object because it return a ZoneTask(which is patched by zone.js), so it will cause the error.

I don't think so:

from the docs:

To schedule execution of a one-time callback after delay milliseconds. Returns a timeoutObject for possible use with clearTimeout(). Optionally you can also pass arguments to the callback.

let's try it:

$ node -v
v0.10.29
$ node -e "console.log(setTimeout(function (){}, 10));"
{ _idleTimeout: 10,
  _idlePrev: 
   { _idleNext: [Circular],
     _idlePrev: [Circular],
     msecs: 10,
     ontimeout: [Function: listOnTimeout] },
  _idleNext: 
   { _idleNext: [Circular],
     _idlePrev: [Circular],
     msecs: 10,
     ontimeout: [Function: listOnTimeout] },
  _idleStart: 1492251774171,
  _onTimeout: [Function],
  _repeat: false }

JiaLiPassion · 2017-04-15T12:12:37Z

@manuelschneider , yes, you are right, in v0.10, it return a TimerObject, and it doesn't have any id like property.

So I think your initial solution is ok because setTimeout really return a TimerObject not an integer, I think you just need to add test code to simulate such case to complete this PR.

manuelschneider · 2017-04-15T22:45:21Z

How do I run tests with node v0.10?

$ npm run test-node

> zone.js@0.8.5 test-node 
> gulp test/node


gulpfile.js:17
const os = require('os');
^^^^^

JiaLiPassion · 2017-04-15T22:47:48Z

@manuelschneider , I think you just need to add a test case in test/common/setTimeout.spec.ts, and write a fake setTimeout which return a fake timerObject to override global setTimeout and test if setTimeout and clearTimeout work and recover the setTimeout to original one after your test.

manuelschneider · 2017-04-16T10:12:43Z

I don't get it. You want me to patch the patched setTimeout with a fake one? Why?

The existing testcases test if a timeout can be scheduled and canceled. In my opinion that's exactly what is needed to make sure the changes don't break something.

My change should not have modified previously existing behaviour. It simply reduced the usage of tasksByHandleId to the cases, when handleId is an integer. It does not modify data.handleId.
tasksByHandleId never worked in node before:

$ node -v
v7.8.0
$ node -e 'let x={a:1}; let y={b:2}; let z={c:3};x[y]="yy"; x[z]="overrides yy"; console.log(x);'
{ a: 1, '[object Object]': 'overrides yy' }

=> Objects still get stringified in newer node versions, when used as key. Which makes them
practically useless for that. The difference is that older node versions threw an exception instead of
silently stringifying all objects to '[object Object]'.

As far as I understand it, the patched setTimeout always returns a task. But when data.handleId is
an integer, toString() from zone.ts (line 1165) returns the integer (see #739). In node data.handleId has always been an object, which is why the tests in setTimeout.spec, which are checking the 'return value' are skipped for node.

I don't now, but for now I can't think of a useful test to my patch apart from the already existing ones.

JiaLiPassion · 2017-04-16T11:08:28Z

@manuelschneider , in current cases, it can only make sure the changes not break anything, I mean we should simulate node v0.10 behavior to test if global.setTimeout return a object insteadof integer, our logic should also work.

the case should like

const originalSetTimeout = global.setTimeout;
global.setTimeout = function() {
    const  originalResult = originalSetTimeout.apply(this, arguments);
   const fakeResult = {
      timer: originalResult;
    }
    return fakeResult;
}
const originalClearTimeout = global.clearTimeout;
global.clearTimeout = function(timerObject) {
    return originalClearTimeout.apply(this, [timerObject.timer]);
}

Zone.current.fork({
   name: 'test',
   scheduleTask: (...) => {
      ...
     // some spy here
   },
   cancelTask:(...) => {
     // some spy here
    }
}).run(() => {
    const timerId = setTimeout(() => {
       // some other spy here
    }, 100);
    clearTimeout(timerId);
});

expect(scheduleSpy).toHaveBeenCalled();
expect(cancelSpy).toHaveBeenCalled();
expect(timeoutCallbackSpy).toHaveBeenCalled();

global.setTimeout = originalSetTimeout;
global.clearTimeout = originalClearTimeout;

manuelschneider · 2017-04-17T19:40:20Z

I still don't see how this should test handling the exceptions?

JiaLiPassion · 2017-04-18T03:30:54Z

@manuelschneider , in my understandings, we should test from those 2 views.

the new code should not break anything, the current test cases can cover this one.
the new code can still work for the error case (here means the node v0.10 behavior, if setTimeout return Object instead of an integer), so we add some cases to simulate this case.

What do you think?

manuelschneider · 2017-04-18T05:13:07Z

the new code can still work for the error case (here means the node v0.10 behavior, if setTimeout return Object instead of an integer), so we add some cases to simulate this case.

This is still the case in newer node versions:

$ node -v
v7.8.0
$ node -e 'console.log(setTimeout(function() {}, 50))'
Timeout {
  _called: false,
  _idleTimeout: 50,
  _idlePrev: 
   TimersList {
     _idleNext: [Circular],
     _idlePrev: [Circular],
     _timer: Timer { '0': [Function: listOnTimeout], _list: [Circular] },
     _unrefed: false,
     msecs: 50,
     nextTick: false },
  _idleNext: 
   TimersList {
     _idleNext: [Circular],
     _idlePrev: [Circular],
     _timer: Timer { '0': [Function: listOnTimeout], _list: [Circular] },
     _unrefed: false,
     msecs: 50,
     nextTick: false },
  _idleStart: 34,
  _onTimeout: [Function],
  _timerArgs: undefined,
  _repeat: null }

fix: don't save handleIds for node #736

fix: don't save handleIds for node #736

Uh oh!

Conversation

manuelschneider commented Apr 13, 2017

Uh oh!

googlebot commented Apr 13, 2017

Uh oh!

googlebot commented Apr 13, 2017

Uh oh!

manuelschneider commented Apr 13, 2017

Uh oh!

googlebot commented Apr 13, 2017

Uh oh!

googlebot commented Apr 13, 2017

Uh oh!

JiaLiPassion left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manuelschneider commented Apr 14, 2017

Uh oh!

JiaLiPassion commented Apr 14, 2017

Uh oh!

manuelschneider commented Apr 15, 2017

Uh oh!

JiaLiPassion commented Apr 15, 2017

Uh oh!

manuelschneider commented Apr 15, 2017

Uh oh!

JiaLiPassion commented Apr 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

manuelschneider commented Apr 15, 2017

Uh oh!

JiaLiPassion commented Apr 15, 2017

Uh oh!

manuelschneider commented Apr 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JiaLiPassion commented Apr 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

manuelschneider commented Apr 17, 2017

Uh oh!

JiaLiPassion commented Apr 18, 2017

Uh oh!

manuelschneider commented Apr 18, 2017

Uh oh!

JiaLiPassion commented Apr 18, 2017

Uh oh!

JiaLiPassion commented Apr 18, 2017

Uh oh!

manuelschneider commented Apr 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

JiaLiPassion left a comment •

edited

Loading

JiaLiPassion commented Apr 15, 2017 •

edited

Loading

manuelschneider commented Apr 16, 2017 •

edited

Loading

JiaLiPassion commented Apr 16, 2017 •

edited

Loading

manuelschneider commented Apr 18, 2017 •

edited

Loading