Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Commit

Permalink
Browse files Browse the repository at this point in the history
fix(tasks): do not drain the microtask queue early.
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.
  • Loading branch information
juliemr authored and mhevery committed Aug 19, 2016
1 parent 5e3c207 commit d4a1436
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 4 deletions.
14 changes: 10 additions & 4 deletions lib/zone.ts
Expand Up @@ -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__;

Expand Down Expand Up @@ -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--;
}
};
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -927,7 +934,6 @@ const Zone: ZoneType = (function(global: any) {
}
}
_isDrainingMicrotaskQueue = false;
_drainScheduled = false;
}
}

Expand Down
42 changes: 42 additions & 0 deletions test/browser/element.spec.ts
Expand Up @@ -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,
Expand Down
27 changes: 27 additions & 0 deletions test/common/zone.spec.ts
Expand Up @@ -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 () {
Expand Down

0 comments on commit d4a1436

Please sign in to comment.