[WIP] feat(async): fix #740, use async hooks to wrap nodejs async api

[JiaLiPassion]

fix #740, use new async_hooks to wrap nodejs instead of monkey-patch all native APIs.

The basic idea is in async_hooks, there are 4 hook apis.

init
before
after
destroy

in this PR, the API will do those work.

1. init

init will call Zone.scheduleTask to schedule a ZoneTask

2. before

before will call Zone.beforeRunTask to create a new ZoneFrame and other work just like Zone.runTask

3. after

after will call Zone.afterRunTask to go back to parent ZoneFrame and do other work just like Zone.runTask

4. destroy

try to cancelTask if not cancelled.

5. Error

use process.unCaughtException to redirect to ZoneDelegate.handleError.

this PR still need a lot of work to do, so I post the PR here just ask for review.

@mhevery , could you review that node_asynchooks.ts the whether the idea is ok or not? Thank you!

[mhevery]

Why do we need to disable the tests? I would imagine that the tests should still work even with async_hook?

[JiaLiPassion]

@mhevery , sorry, I just began to debug those test codes, all tests should be able to run with async_hook, but some tests need to be modified.

So please ignore those test case files.

And I found to make all tests work may cause some time and some test will still wait nodejs to make some changes, so I just make the PR to let you review the zone.ts and zone_async_hooks core code.

I will finish those test codes and let you review later.

[mhevery]

Why are these needed? Is it because the flush() no longer works?

[JiaLiPassion]

yes, just like the above one, I am just began to debug those tests, I will finish modifying those tests and let you review. Please ignore those files for now.

[mhevery]

I am not a fan of beforeRunTask and afterRunTask, but I do understand why you need them. But I think it would mean that we should also add them to ZoneDelegate? Or are you planning on making them private?

How do we communicate to the developer that this particular Task can't be wrapped? It never has runTask executed on it. It can never be in fakeAsynczone.

[JiaLiPassion]

yes, those APIs are private.

And I will check fakeAsync , I think the runTask will be called because in fakeAsyncZone, the callback is task.invoke and it will finally call task.zone.runTask. I will check it.

[mhevery]

Why are these needed here? Seems out of place

[JiaLiPassion]

yes, those will be deleted finally, I just want to make some current test case pass easily, and I can debug them easily. I will remove them later.

[mhevery]

I see you are making them private, but than we should somehow hide them from the public API.

[JiaLiPassion]

I want to confirm I understand hiding from public API correctly.

We should hide beforeRunTask, runTask, afterRunTask from Zone if we use async_hooks, because beforeRunTask and afterRunTask should only be called from async_hooks, so runTask should never be called from user code, otherwise it will also call beforeRunTask and afterRunTask and cause issue.

so in async_hooks mode, all those 3 methods are not available, is that right?

For compatibility with non async_hooks, should we just add a flag to check we are using async_hooks or not, and if we are using async_hooks, we just return when user call runTask, is that ok? I will make a commit to describe it!

[mhevery]

Task has data. We should put BeforeRunTaskStatus there, rather than monkey patching it on Task directly.

[JiaLiPassion]

Got it, I will change it.

[JiaLiPassion]

waiting for nodejs async_hooks to provide parent promise information.

nodejs/node#13302

currently async_hooks can't tell the difference between a newly created promise
is a chained one or not.

for example

const p = new Promise ( resolve => {
    process._rawDebug('execute p');
    resolve(1);} );

const p1 = p.then(r => ...);

both p and p1 will trigger async_hooks 's init callback, but only
p1 should trigger ZoneSpec 's onScheduleTask callback.

@mhevery , please help to check this is correct or not, thank you !

I made a PR to get parent promise information.

nodejs/node#13367

This PR only correct the init hook triggerId behavior, about how to get parent promise in init callback is still under discuss....

And it will be implemented soon.

nodejs/node#13367 (comment)
nodejs/node#13452

[matthewloring]

@JiaLiPassion We are also interested in getting Zones working on top of async hooks avoiding monkey patching if possible and would be happy to help. It looks like some zone APIs require a handle to the callback associated with an async task (onIntercept, onInvoke, etc). Did you have thoughts on how these callbacks could be observed using async hooks without monkey patching?

[JiaLiPassion]

@matthewloring, thank you for reply!

We are also interested in getting Zones working on top of async hooks avoiding monkey patching if possible and would be happy to help

thank you and I definitely need your help on this changes.

It looks like some zone APIs require a handle to the callback associated with an async task (onIntercept, onInvoke, etc). Did you have thoughts on how these callbacks could be observed using async hooks without monkey patching?

Yes, you are right, currently I have not figure out how to get the callback handle in onIntercept, onInvoke, onInvokeTask of ZoneSpec. It seems async_hooks not provide callback handle API.

• In my very earlier idea, I only want to use async_hooks to resolve native async/await not handled by zone.js problem

await ayncCall(); // it will use native promise so will not use monkey-patched zoneAwarePromise

and other API such as timer, fs still need to be monkey-patched.

But if async_hooks can provide the full information zone.js needed, I think use async_hooks is a much more better solution.

And if async_hooks can't provide callback handle, I think we can also discuss to provide a non-callback handle onInvoke callback.

@mhevery , if we use async_hooks, do you think it is ok to provide onInvokeTask callback like this?

// task.callback will be undefined.
onInvokeTask(delegate, currentZone, targetZone, task);

[JiaLiPassion]

@mhevery , @matthewloring , other things that may change when use async_hooks instead of monkey-patch.

1. Promise, onScheduleTask timing.

const zone = Zone.current.fork({
  name: 'promise',
  onScheduleTask: (...) => {
    console.log('scheduleTask', task.source);
    return delegate.scheduleTask(targetZone, task);
   }
});

zone.run(() => {
  const p1 = new Promise(resolve => { setTimeout(() => {
    console.log('resolve promise p1');
    resolve();
  }, 100)});

  const p2 = p1.then(v => return v);
});

in the current monkey-patch ZoneAwarePromise version.
the output will be

resolve promise p1
scheduleTask promise // this is p2

and if we use async_hooks, the output will be

scheduleTask promise // this is p2
resolve promise p1

Because in ZoneAwarePromise, a microTask will be scheduled when it's parent promise resolved/rejected, and in async_hooks, a microTask will be scheduled when a child promise was created(when promise.then was called).

2. onHandleError will be a little risky.

just like @matthewloring said, we have no callback handle, so we can't

try {
  task.callback.apply(target, args);
} catch (err) {
  zone.handleError(err);
}

we have to use

  process.on('uncaughtException', (err: any) => {

     // get current zone
     Zone.current.handleError(err);
  }

[JiaLiPassion]

@matthewloring , continue to discuss about the callback handle, in my understanding, async_hooks not like monkey-patch, it is only notification, it will not allow the hook listener to control the lifecycle of the async callback.

so in async_hooks,

setTimeout(() => {
    console.log('timeout callback')'
});

will only trigger before and after, and the callback will be invoked anyway.

but in zone.js monkey-patch,

onInvokeTask(...) {
  // zone can decide invoke callback or not
}

So I think from the design, async_hooks will not support pass callback handle, even it does, for example pass the callback handle in before hook, the hook listener can only reference the callback handle information, but can't control the callback will be invoked or not, is that correct?

[matthewloring]

That is correct, async hooks allows for observation of async events while zones relies on being able to reschedule async work. This was my primary concern with refactoring zones to use async hooks. One possibility would be to have two "modes" supported by zones. A fast, observation-only mode based on async hooks that supported async/await and a slower, monkey-patching based mode which allows rescheduling async tasks but does not have async/await support. This might be too invasive a change (@mhevery what are your thoughts?) but would that address your use case @JiaLiPassion?

[JiaLiPassion]

@matthewloring

One possibility would be to have two "modes" supported by zones. A fast, observation-only mode based on async hooks that supported async/await and a slower, monkey-patching based mode which allows rescheduling async tasks but does not have async/await support.

Yeah, your suggestion could be a good solution.

So it will be zone.js provide 2 modes, and let application select by some configurations.

• fast observation-only mode, only provide before, after callback, it will resolve the most use cases, but application can't not control the callback handle.

• original monkey-patch mode without native async/await support.

And about the original monkey-patch mode, I think maybe we can use promise async_hooks to support native async/await. so it will be a mix-mode. most API will be monkey-patched and promise will be monkey-patched and useasync_hooks to resolve native async/await issue.

[mhevery]

I am on vacation till end of month. But your discussion sounds reasonable to me.

[ofrobots]

If it has two 'modes', would it make sense to split the library into two?

[JiaLiPassion]

@ofrobots, yeah, maybe build a zone.js and zone-async-hooks maybe more clear,but I am not sure about this one...

[mhevery]

@ofrobots, yeah, maybe build a zone.js and zone-async-hooks maybe more clear,but I am not sure about this one...

@JiaLiPassion we could already do this with Zone.__load_patch api.

[JiaLiPassion]

@mhevery

@JiaLiPassion we could already do this with Zone.__load_patch api.

So you mean we load

Zone.__load_patch('node'); 
Zone.__load_patch('async_hooks`);

and add some checks to prevent both them are loaded, is that correct?

[mhevery]

@JiaLiPassion that is what I had in mind

[JiaLiPassion]

@mhevery , got it, I will continue to work on it after the performance one is done.

[matthewloring]

@mhevery Is it expected behavior for zones to only support a subset of its API depending on the set of patches loaded using Zone.__load_patch?

[JiaLiPassion]

@matthewloring ,

Is it expected behavior for zones to only support a subset of its API depending on the set of patches loaded using Zone.__load_patch?

In my understanding, it is, you can find all the modules which are loaded by zone.js with __load_patch here, you can disable any of them.

[matthewloring]

@JiaLiPassion Looking at the list of patches, it seems like they make zones aware of more async constructs but still support the full zones API. That is, the set of tasks that zones are aware of increases but if I attach an onIntercept function it will still behave correctly for all tasks. If an async hooks plugin were added and a user attempts to reschedule all task callbacks using onIntercept then all tasks corresponding to async hook events will ignore the results of the onIntercept callback if I understand correctly.

[JiaLiPassion]

@matthewloring , I want to make sure I understand your meaning correctly.

1. current zone.js onIntercept callback will not fire for tasks. It will only fire when you use zone.wrap like this.

const foo = function() {
  console.log('foo');
};

const fooInZone = Zone.current.fork({
  name: 'test',
  onIntercept: function (..., zone, callback) {
     console.log('callback intercepted');
     return callback;
   }
}).wrap(foo);

fooInZone();

And in this code, onIntercept will be triggered.
But for other ZoneTasks, such as setTimeout, Promise, XMLHttpRequest, the onIntercept will not be invoked.

const fooInZone = Zone.current.fork({
  name: 'test',
  onIntercept: function (..., zone, callback) {
     console.log('callback intercepted');
     return callback;
   }
}).run(() => {
  setTimeout(() => {
}, 1000);
});

In this code, onIntercept will not be triggered.

a user attempts to reschedule all task callbacks

About reschedule, if you talk about onIntercept, do you mean to return a new callback other than the original one?

const foo = function() {
  console.log('foo');
};

const fooInZone = Zone.current.fork({
  name: 'test',
  onIntercept: function (..., zone, callback) {
     console.log('callback intercepted');

    // here return a new callback
     return function() {
       console.log('new foo');
     };
   }
}).wrap(foo);

fooInZone();

do you mean some use case like the above code?

And for ZoneTask (setTimeout, promise...), zone.js currently provide a way to reschedule in new zone. For example,

const barZone = Zone.current.fork({
  name: 'bar'
});
const fooInZone = Zone.current.fork({
  name: 'foo',
  onScheduleTask: function (zoneDelegate, currentZone, targetZone, task) {
      if (task.source === 'something') {
       // I want the task to be scheduled in bar zone
       task.cancelScheduleRequest();
       return barZone.scheduleTask(task);
      }
   }
}).wrap(foo);

fooInZone();

And I would like to confirm

If an async hooks plugin were added and a user attempts to reschedule all task callbacks using onIntercept then all tasks corresponding to async hook events will ignore the results of the onIntercept callback if I understand correctly.

what is the ignore the results you mean?

[JiaLiPassion]

@mhevery , @matthewloring

I will summarize the known issues here.

1. native async/await issue
2. async_hooks only support readonly notification mode
3. async_hooks onHandleError issue

1. Native async/await issue.

Now with async_hooks API, we don't know when to scheduleMicroTask. Even async_hooks expose the PromiseHooks API, https://docs.google.com/document/d/1rda3yKGHimKIhg5YeoAmCOtyURgsbTH_qaYR79FELlk, we still don't know when.

The issue can be described as

case1: promise's parent is an resolved promise.

const p1 = Promise.resolve();

const p2 = p1.then(r => r);

In this case, current async_hooks API is ok, I can just schedule a microTask in zone.js in the init callback.

function init(id: number, provider: string, triggerId: number, parentHandle: any) {
    if (provider === PROMISE_PROVIDER) {
      if (!parentHandle) {
        return;
      }
      const parentId = parentHandle.parentId;
      if (!parentId) {
        return;
      }

      const task = Zone.current.scheduleMicroTask(PROMISE_SOURCE, noop, null, noop);
      promiseTasks[id] = task;
    }
  }

case2: the promise's parent is an unresolved promise.

const p1 = new Promise(() => {}); // This will never be resolved
const p2 = new Promise((resolve) => setTimeout(resolve, 1000, p1)); // resolve p2 with p1 after 1s

In this case, the microTask should not be scheduled in resolve hook, when the resolve(p1) is called, because p1 is still unresolved, the microtask should be scheduled when p1.resolve is called(in this case, p1 is never resolved), so I wonder is that possible to expose the PromiseHook.resolve callback through async_hooks?

And only expose PromiseHook.resolve seems can't resolve this issue, I also want to know what state promise in to check the following case.

const p1 = new Promise(() =>{});
const p2 = new Promise((resolve) => setTimeout(resolve, 1000, p1)); // resolve p2 with p1 after 1s

in this case, resolve (p2) hook will be called, but p2 is still in pending state, so what I really want is when promise is really resolved/rejected through some hook API.

2. Async hooks only support notification mode.

As we discussed, async hooks can't get the control of callback, so we need to implement a notification version of zone.js.

3. onHandleError

Because we can't get the control of callback, so we are not be able to do monkey-patch like
the code below.

try {
  task.callback.apply(target, args);
} catch (err) {
  zone.handleError(err);
}

we have to use

  process.on('uncaughtException', (err: any) => {

     // get current zone
     Zone.current.handleError(err);
  }

It will become a little risky.

[AndreasMadsen]

so I wonder is that possible to expose the PromiseHook.resolve callback through async_hooks?

Hello from async_hooks. This is something we are discussing, please see nodejs/node#13437.

[JiaLiPassion]

@AndreasMadsen , thank you very much, and to implement zone.js with promiseHooks, the current PromiseHooks API seems not enough, not only I want to know when resolve is called, but also I need to know when the promise is really resolved/rejected so I can schedule microTasks for all the chained promises (promise.then).

const p1 = new Promise(() => {}); // This will never be resolved
const p2 = new Promise((resolve) => setTimeout(resolve, 1000, p1)); // resolve p2 with p1 after 1s

In this case, the microTask should not be scheduled in resolve hook, when the resolve(p1) is called, because p1 is still unresolved, the microtask should be scheduled when p1.resolve is called(in this case, p1 is never resolved).

in my understanding the v8 promise.then and resolve implementation looks like.

function then(onFulfilled, onRejected) {
  const chainedPromise = new Promise();
  if (this.state == UNRESOLVED) {
    // current promise not resolved, keep callback into a list
    this[thenList].push(chainPromise, onFulfilled, onRejected);
  } else {
    // current promise is resolved, we can enqueue the chained promise to microTask queue
    // here we really schedule an async microTask
   // in zone.js I need to know when promise get here
    enqueue(chainedPromise);
   }
}

function resolvePromise(promise, value) {
    // here the resolve hook will be triggered
   resolveHook(promise);
   if (isPromise(value) && value.state == UNRESOLVED) {
     // promise still unresolved
     value.then(...);
     return;
   } 
   
   // value is thenable, so promise is not really resolved
   if (isThenable(value)) {
     value.then(...);
     return;
   }

   chainedPromiseList = getThenList(promise);

   // promise was really resolved, we enqueue to start a microTask for chained promise
   // in zone.js I need to know when promise get here
   chainedPromiseList.forEach(chainedPromise => enqueue(chainedPromise))
}

**so I wonder is that possible to

• expose the PromiseHook.resolve callback through async_hooks and in the hook we can get the state of promise (unresolved, resolved, rejected)**

[AndreasMadsen]

I think you are using a technical vocabulary that is specific to zone.js, so I don't really understand what you are saying about microTask and resolve hook. I will try and comment on the specific questions:

so I wonder is that possible to expose the PromiseHook.resolve callback through async_hooks

yes, this is what nodejs/node#13437 is about.

and in the hook we can get the state of promise (unresolved, resolved, rejected).

The resource.promise value contains the promise object itself. Unfortunately, there is no public API for checking the state of that promise object. However, it is possible:

const binding = process.binding('util');
const state2string = new Map([
  [binding.kPending, 'pending'],
  [binding.kFulfilled, 'fulfilled'],
  [binding.kRejected, 'rejected']
]);

const [state, result] = binding.getPromiseDetails(resource.promise);
const readableState = state2string.get(state);
console.log(readableState); // fulfilled

I think this was explained in nodejs/node#14038 (comment)

I have no idea if binding.getPromiseDetails is something we want to make public, it is not really my area. But if we do it won't be though async_hooks as it is too promise specific.

[JiaLiPassion]

@AndreasMadsen , thank you very much!

I think you are using a technical vocabulary that is specific to zone.js, so I don't really understand what you are saying about microTask and resolve hook. I will try and comment on the specific questions:

sorry for poor explanation.

const p1 = new Promise(() => {}); // This will never be resolved
const p2 = new Promise((resolve) => setTimeout(resolve, 1000, p1)); // resolve p2 with p1 after 1s

in this case, resolve p2 with p1 after 1s will trigger PromiseResolve hooks, but p1 and p2 are still unresolved. for zone.js, I need a trigger to tell that the promise is really resolved, not just a trigger to tell that resolvePromise function was called.

And about get state of promise.

const binding = process.binding('util');
const state2string = new Map([
  [binding.kPending, 'pending'],
  [binding.kFulfilled, 'fulfilled'],
  [binding.kRejected, 'rejected']
]);

const [state, result] = binding.getPromiseDetails(resource.promise);
const readableState = state2string.get(state);
console.log(readableState); // fulfilled

Thank you for the information,

• will this API be gone in nodejs future version ?
• will this get state call have performance impact?

[AndreasMadsen]

I need a trigger to tell that the promise is really resolved, not just a trigger to tell that resolvePromise function was called.

I see, have you experimented with using resource.promise.then()?

will this API be gone in nodejs future version ?

You should assume so. If you have a good use case, you should open an issue in node.js where you request this feature is made public.

will this get state call have performance impact?

Most C++ calls have, but you will have to benchmark it to be sure. The internal function is used in util.inspect, which is not something we are heavily optimizing as it is mostly used for debugging.

[JiaLiPassion]

@AndreasMadsen

I see, have you experimented with using resource.promise.then()?

the timing of promise.then() is late, and a really want is after parent promise resolved and before enqueue the chained promise to micro tasks queue.

in my understandings, the code look like this.

resolvePromise(promise, value) {
   promise.state = RESOLVED;
   // here I want a trigger.
   trigger(PROMISE_RESOLVED);
   // push the chained promise to microTask (process.tick?) queue.
   enqueueToMicroTask(getChainedPromises(promise));
}

And promise.then(callback), the callback will be executed in microTask queue (process.tick?), so I really want the trigger before the then callback was pushed into the microTask queue.

will this API be gone in nodejs future version ?
You should assume so. If you have a good use case, you should open an issue in node.js where you request this feature is made public.

Got it, I will try to make some cases to describe my requirements.

will this get state call have performance impact?
Most C++ calls have, but you will have to benchmark it to be sure. The internal function is used in util.inspect, which is not something we are heavily optimizing as it is mostly used for debugging.

Got it, thank you, I will make benchmark to test it.

[jpsfs]

Any news on this?

[JiaLiPassion]

@jpsfs, currently I am waiting for https://bugs.chromium.org/p/v8/issues/detail?id=6862 to resolve the promise issue.

[JiaLiPassion]

Update on 2018/01/04

Support es2017 async/await

Because currently asynchooks promiseResolve hooks can't tell whether the promise is really resolved or not. I can't use promiseResolve to implement native async/await, I use another approach. which is override native promise.then and use process.nextTick to schedule another microTask.

the idea can be described as the following code.

init(id, triggerId, parentHandle) {
   const promise = parentHandle.promise;
   idPromises[id] = Zone.current; // save id and zone mapping
    promise.then = function(onResolve: any, onReject: any) {
      const task = zone.scheduleMicroTask(PROMISE_PROVIDER, noop, null, noop);
      isPromiseTick = true;
      process.nextTick(() => {
        task.zone.runTask(task, null, null);
        originalThen.call(this, onResolve, onReject);
      });
    };
}

before(id) {
  const zone = getZoneById(); 
  setCurrentZoneFrame(zone); // retrieve the zone by id
}

after(id) {
  setCurrentZoneFrame(null); // leave zone
}

and consider the following use case.

    const zone = Zone.current.fork({name: 'promise'});
    async function asyncOutside() {
      return 'asyncOutside';
    }

    zone.run(async() => {
      const outside = await asyncOutside();  
      expect(outside).toEqual('asyncOutside');
      expect(Zone.current.name).toEqual(zone.name);
  })

with es2017 target compile and the new asynchooks module, the invoke sequence will be.

1. zone.run
2. await asyncOutside();
3. a new promise was initialized
4. asynchooks.init was invoked, the aysncId for example 100 and Zone.current (promise) will be kept in memory.
5. zone.run finished, Zone.current become <root>
6. await is about to finished, asynchooks.before was invoked with id 100. so we recover the zone with promise.
7. expect(outside).toEqual('asyncOutside'); expect(Zone.current.name).toEqual('promise') will be executed in promise zone.
8. asynchooks.after is invoked with id 100, then we exit promise zone and go back to it's parent zone <root>.

Because promiseResolve is not support telling whether the promise is really resolved, we still need ZoneAwarePromise, and when promiseResolve can tell more information about promise status, we maybe able to remove ZoneAwarePromise.

  gulp test/node/es2017

will run native async/await test.

[JiaLiPassion]

Update on 2018/01/04

Support asynchooks.

Use asynchooks instead of monkey-patch,for example fs, setTimeout will use native asynchooks, but not all APIs, EventEmitter, console still need to be patched.

in this PR,

  gulp test/node/asynchooks

will run all tests for node in asynchooks mode.

Still need to tuning performance and add test cases for all asynchooks providers.

[AndreasMadsen]

@JiaLiPassion Be aware that the V8 team would like to remove parentHandle.promise, but I will try and push for promiseResolve showing the "resolved" value as a requirement for that (it is much easier if you are actually using parentHandle.promise).

The tracking won't be as easy as getting a specific event, but I think it is much easier for the V8 team to implement and I would like to avoid adding another promise-specific event to async_hooks. With the "resolved" value, you can simply check if it is a promise. Since you are already tracking that promise, you can simply see if and how that was resolved.

[JiaLiPassion]

@AndreasMadsen , thank you for the information. If a promiseResolve call with a value parameter, it will solve my problem, I don't need to schedule an additional microTask to simulate a micro async operation.

And asynchooks are not designed to handle EventEmitter, is that correct? because in zone.js, EventEmitter.on is also some kind of async operation.

[AndreasMadsen]

And asynchooks are not designed to handle EventEmitter, is that correct? because in zone.js, EventEmitter.on is also some kind of async operation.

That is correct. EventEmitter's are just an interface they don't actually task an asynchronous operation themselves. They are also pretty easy to monkey-patch as the API is very fixed. We actually do it for domain in nodecore: https://github.com/nodejs/node/blob/master/lib/domain.js#L400

[JiaLiPassion]

@AndreasMadsen , got it, now zone.js also monkey-patch the EventEmitter api to memorize zone information, I will keep it that way.