fix: Support clearing the timeouts with numeric IDs #462

platosha · 2016-09-20T14:07:18Z

Fixes #461

googlebot · 2016-09-20T14:07:20Z

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 · 2016-09-20T14:07:20Z

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.

platosha · 2016-09-20T14:09:34Z

I signed it!

googlebot · 2016-09-20T14:09:35Z

CLAs look good, thanks!

googlebot · 2016-09-20T14:09:35Z

CLAs look good, thanks!

mhevery · 2016-10-01T02:17:08Z

You need a test to demonstrate the issue.

mhevery · 2016-10-01T02:17:59Z

lib/common/timers.ts

@@ -16,6 +16,8 @@ export function patchTimer(
  setName += nameSuffix;
  cancelName += nameSuffix;

+  var tasksByHandleId: Array<Task> = [];


This array will grow forever. I think using a hash may be better here.

mhevery · 2016-10-01T03:26:17Z

lib/common/timers.ts

+    if (task && typeof task === 'number') {
+        var handleId = <number>task;
+        task = tasksByHandleId[handleId];
+        tasksByHandleId[handleId] = undefined;


also this also has to be cleared when the task expires

platosha · 2016-10-04T14:51:42Z

@mhevery added the test.

vicb · 2016-10-04T17:04:39Z

lib/common/timers.ts

@@ -19,14 +19,21 @@ export function patchTimer(window: any, setName: string, cancelName: string, nam
  setName += nameSuffix;
  cancelName += nameSuffix;

+  var tasksByHandleId: Object = {};


please use const

vicb · 2016-10-04T17:05:15Z

test/common/setTimeout.spec.ts

+      return;
+    }
+
+    var testZone = Zone.current.fork(Zone['wtfZoneSpec']).fork({ name: 'TestZone' });


const, const, const, let instead of var

vicb · 2016-10-06T01:38:35Z

test/common/setTimeout.spec.ts

+      const task: Task = <any>setTimeout(spy, 0);
+      const cancelId: number = <any>task;
+      clearTimeout(0 + cancelId);
+      setTimeout(function () {


should you set a timeout of ie 10 here ?
I don't think there is any guarantee on the order for the same timeout value (even if most impl should work as expected)

Right.

Done. Set the second timeout of 1.

Please note that the same point also applies to another test in this file as well, see https://github.com/platosha/zone.js/blob/02d0784bb9e9cfb44d2eaa32c12be538370b0a0b/test/common/setTimeout.spec.ts#L57

could you please also modify the other occurence ?

Done, two places. Modified in a separate commit.

vicb · 2016-10-06T20:43:26Z

lib/common/timers.ts

@@ -60,6 +67,9 @@ export function patchTimer(window: any, setName: string, cancelName: string, nam
  clearNative =
      patchMethod(window, cancelName, (delegate: Function) => function(self: any, args: any[]) {
        var task: Task = args[0];
+        if (task && typeof task === 'number') {
+          task = tasksByHandleId[<number>task];
+        }


should be equivalent to:

if (typeof task === 'number') { task = tasksByHandleId[task]; }

Done.

Can’t drop the <number> type casting in the tasksByHandleId[<number>task] index expression, though, the TypeScript compiler gives error.

vicb · 2016-10-07T17:58:01Z

lib/common/timers.ts

  function scheduleTask(task: Task) {
    const data = <TimerOptions>task.data;
-    data.args[0] = task.invoke;
+    data.args[0] = function() {
+      task.invoke.apply(this, arguments);


should this be task here ?
or task.invoke(...arguments) ?

No.

Before the change, task.invoke was used as a timer callback. Timer callbacks are executed with some other context: this is a Timeout instance in Node.js, window in browers, undefined in strict mode.

Within this change, we use another function as a timer callback, but in the function we still call task.invoke with the original timer callback context. As it was called before the change.

Note, that the invoke method of ZoneTask, which is called here, also passes the context further, so it ends as the original user-provided task callback context. Changing it is not good. Consider:

setTimeout(function() { console.log(this); // should log `window` in browsers, not a `ZoneTask` instance or anything else });

This doesn’t fail any Zone.js tests, though. Seems like there is a lack of tests for the timer callbacks context.

vicb · 2016-10-07T17:58:38Z

lib/common/timers.ts

@@ -60,6 +67,9 @@ export function patchTimer(window: any, setName: string, cancelName: string, nam
  clearNative =
      patchMethod(window, cancelName, (delegate: Function) => function(self: any, args: any[]) {
        var task: Task = args[0];
+        if (typeof task === 'number') {
+          task = tasksByHandleId[<number>task];


remove <number> should be implied by the typeof check above

Fixes angular#461

Some of the setTimeout tests use multiple timers and expect them to execute in a particular order. Use explicit delay in those places to reflect and guarantee the execution order.

googlebot added the cla: no label Sep 20, 2016

googlebot added cla: yes and removed cla: no labels Sep 20, 2016

mhevery force-pushed the master branch from c4da1fa to 832baa0 Compare September 30, 2016 22:22

mhevery suggested changes Oct 1, 2016

View reviewed changes

mhevery force-pushed the master branch from 570cd5a to 16be7f9 Compare October 1, 2016 02:33

mhevery reviewed Oct 1, 2016

View reviewed changes

platosha force-pushed the fix/timeoutIds branch from 8726c08 to 9991b5c Compare October 4, 2016 14:49

vicb reviewed Oct 4, 2016

View reviewed changes

platosha force-pushed the fix/timeoutIds branch from 9991b5c to c23a1b2 Compare October 5, 2016 09:48

vicb reviewed Oct 6, 2016

View reviewed changes

platosha force-pushed the fix/timeoutIds branch from c23a1b2 to 02d0784 Compare October 6, 2016 12:41

vicb reviewed Oct 6, 2016

View reviewed changes

platosha force-pushed the fix/timeoutIds branch from 02d0784 to 5fcb1d0 Compare October 7, 2016 07:47

vicb reviewed Oct 7, 2016

View reviewed changes

platosha added 2 commits October 12, 2016 12:36

fix: support clearing the timeouts with numeric IDs

bb84c11

Fixes angular#461

test(setTimeout): use explicit timeout delays where order matters

3dd4f06

Some of the setTimeout tests use multiple timers and expect them to execute in a particular order. Use explicit delay in those places to reflect and guarantee the execution order.

platosha force-pushed the fix/timeoutIds branch from 5fcb1d0 to 3dd4f06 Compare October 12, 2016 09:36

mhevery closed this in abaac6f Oct 12, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Support clearing the timeouts with numeric IDs #462

fix: Support clearing the timeouts with numeric IDs #462

platosha commented Sep 20, 2016

googlebot commented Sep 20, 2016

googlebot commented Sep 20, 2016

platosha commented Sep 20, 2016

googlebot commented Sep 20, 2016

googlebot commented Sep 20, 2016

mhevery commented Oct 1, 2016 •

edited

Loading

mhevery Oct 1, 2016

platosha Oct 4, 2016

mhevery Oct 1, 2016

platosha Oct 4, 2016

platosha commented Oct 4, 2016

vicb Oct 4, 2016

platosha Oct 5, 2016

vicb Oct 4, 2016

platosha Oct 5, 2016

vicb Oct 6, 2016

platosha Oct 6, 2016

vicb Oct 6, 2016

platosha Oct 7, 2016

vicb Oct 6, 2016

platosha Oct 7, 2016

vicb Oct 7, 2016

platosha Oct 12, 2016

vicb Oct 7, 2016

platosha Oct 12, 2016

fix: Support clearing the timeouts with numeric IDs #462

fix: Support clearing the timeouts with numeric IDs #462

Conversation

platosha commented Sep 20, 2016

googlebot commented Sep 20, 2016

googlebot commented Sep 20, 2016

platosha commented Sep 20, 2016

googlebot commented Sep 20, 2016

googlebot commented Sep 20, 2016

mhevery commented Oct 1, 2016 • edited Loading

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

platosha commented Oct 4, 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

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

mhevery commented Oct 1, 2016 •

edited

Loading