fix(tasks): do not drain the microtask queue early.

juliemr · mhevery · commit d4a143699eb3 · 2016-08-18T22:37:21.000-07:00
Fixes a bug where a event task invoked inside another task
would drain the microtask queue too early. This would mean that microtasks
would be called unexpectedly in the middle of what should have been
a block of synchronous code.
diff --git a/lib/zone.ts b/lib/zone.ts
@@ -504,6 +504,7 @@ const Zone: ZoneType = (function(global: any) {
   if (global.Zone) {
     throw new Error('Zone already loaded.');
   }
+
   class Zone implements AmbientZone {
     static __symbol__: (name: string) => string = __symbol__;
 
@@ -835,10 +836,14 @@ const Zone: ZoneType = (function(global: any) {
       this.callback = callback;
       const self = this;
       this.invoke = function () {
+        _numberOfNestedTaskFrames++;
         try {
           return zone.runTask(self, this, <any>arguments);
         } finally {
-          drainMicroTaskQueue();
+          if (_numberOfNestedTaskFrames == 1) {
+            drainMicroTaskQueue();
+          }
+          _numberOfNestedTaskFrames--;
         }
       };
     }
@@ -869,10 +874,12 @@ const Zone: ZoneType = (function(global: any) {
   let _microTaskQueue: Task[] = [];
   let _isDrainingMicrotaskQueue: boolean = false;
   const _uncaughtPromiseErrors: UncaughtPromiseError[] = [];
-  let _drainScheduled: boolean = false;
+  let _numberOfNestedTaskFrames = 0;
 
   function scheduleQueueDrain() {
-    if (!_drainScheduled && !_currentTask && _microTaskQueue.length == 0) {
+    // if we are not running in any task, and there has not been anything scheduled
+    // we must bootstrap the initial task creation by manually scheduling the drain
+    if (_numberOfNestedTaskFrames == 0 && _microTaskQueue.length == 0) {
       // We are not running in Task, so we need to kickstart the microtask queue.
       if (global[symbolPromise]) {
         global[symbolPromise].resolve(0)[symbolThen](drainMicroTaskQueue);
@@ -927,7 +934,6 @@ const Zone: ZoneType = (function(global: any) {
         }
       }
       _isDrainingMicrotaskQueue = false;
-      _drainScheduled = false;
     }
   }
 
diff --git a/test/browser/element.spec.ts b/test/browser/element.spec.ts
@@ -50,6 +50,48 @@ describe('element', function () {
     button.dispatchEvent(clickEvent);
   });
 
+  it('should not call microtasks early when an event is invoked', function(done) {
+    // we have to run this test in setTimeout to guarantee that we are running in an existing task
+    setTimeout(() => {
+      var log = '';
+      Zone.current.scheduleMicroTask('test', () => log += 'microtask;');
+      button.addEventListener('click', () => log += 'click;');
+      button.click();
+
+      expect(log).toEqual('click;');
+      done();
+    });
+  });
+
+  it('should call microtasks early when an event is invoked', function(done) {
+    /*
+     * In this test we escape the Zone using unpatched setTimeout.
+     * This way the eventTask invoked from click will think it is the top most
+     * task and eagerly drain the microtask queue.
+     *
+     * THIS IS THE WRONG BEHAVIOR!
+     *
+     * But there is no easy way for the task to know if it is the top most task.
+     *
+     * Given that this can only arise when someone is emulating clicks on DOM in a synchronous
+     * fashion we have few choices:
+     * 1. Ignore as this is unlikely to be a problem outside of tests.
+     * 2. Monkey patch the event methods to increment the _numberOfNestedTaskFrames and prevent
+     *    eager drainage.
+     * 3. Pay the cost of throwing an exception in event tasks and verifying that we are the
+     *    top most frame.
+     */
+    global[Zone['__symbol__']('setTimeout')](() => {
+      var log = '';
+      Zone.current.scheduleMicroTask('test', () => log += 'microtask;');
+      button.addEventListener('click', () => log += 'click;');
+      button.click();
+
+      expect(log).toEqual('click;microtask;');
+      done();
+    });
+  });
+
   it('should work with addEventListener when called with an EventListener-implementing listener', function () {
     var eventListener = {
       x: 5,
diff --git a/test/common/zone.spec.ts b/test/common/zone.spec.ts
@@ -150,6 +150,33 @@ describe('Zone', function () {
       ]);
     });
   });
+
+  describe('invoking tasks', () => {
+    var log;
+    function noop() {}
+
+
+    beforeEach(() => {
+      log = [];
+    });
+
+    it('should not drain the microtask queue too early', () => {
+      var z = Zone.current;
+      var event = z.scheduleEventTask('test', () => log.push('eventTask'), null, noop, noop);
+
+      z.scheduleMicroTask('test', () => log.push('microTask'));
+
+      var macro = z.scheduleMacroTask('test', () => {
+        event.invoke();
+        // At this point, we should not have invoked the microtask.
+        expect(log).toEqual([
+          'eventTask'
+        ]);
+      }, null, noop, noop);
+
+      macro.invoke();
+    });
+  });
 });
 
 function throwError () {
