Asyncwrap more

[trevnorris]

Pull Request check-list

• Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
  this change (including linting)?
• Is the commit message formatted according to CONTRIBUTING.md?
• If this change fixes a bug (or a performance problem), is a regression
  test (or a benchmark) included?

Affected core subsystem(s)

async_wrap

Description of change

Series of commits that update the AsyncWrap API in the effort to make it ready for public API. Significant changes are:

• Add final callback that runs after the nextTickQueue and MicrotaskQueue has been depleted, or if in the process an exception was thrown but was caught by a domain or 'uncaughtException' handler.
• Pass second argument to post and final callbacks that notify user if an exception was thrown and was caught by a domain or 'uncaughtException' handler. If this is true for post then its final callback will not be called.
• setupHooks() now accepts an object instead of a series of arguments.
• If an async wrap hook throws the process will no longer abort. Instead it will clear all domain and 'uncaughtException' handlers to forcefully allow the exception to bubble and exit the process.

R=@AndreasMadsen
R=@bnoordhuis

CI: https://ci.nodejs.org/job/node-test-pull-request/1946/

[trevnorris]

All green except for a single flaky test failure?! Must be an omen this is ready.

[benjamingr]

I think @vkurchatkin had some code proposed using KickNextTick. So pinging.

I'd also love to know where we stand on promises in AsyncWrap.

[AndreasMadsen]

What is the use case for the final hook?

[trevnorris]

@AndreasMadsen Working through the logic of propagating the parent handle asynchronously I realized that the usual case is that nextTick() callbacks are run in the same "asynchronous" execution scope as the callback itself. For example:

var parentId = [];
function pre(id) {
  parentId.push(id);
}
function post(id, didThrow) {
  if (didThrow)
    parentId.length = 0;
}
function final(id, didThrow) {
  if (didThrow)
    parentId.length = 0;
  else
    parentId.pop();
}

The edge case is if a callback in the nextTickQueue throws and is caught by a domain or 'uncaughtException' handler. At which time we store the active parent and the number of callbacks remaining in the nextTickQueue so that parent can be restored and the callbacks can be run appropriately.

Though since this case is an exception the overhead from the additional logic shouldn't be a burden. Especially since the overhead for storing the parent on every new nextTick() would be much greater.

Reason I haven't included it here is because additional API is required to make it useful. I was considering something to the affect of async_wrap.parent_id() to return the active parent's id. Currently the "parent" is only propagated in cases like a new TCP connection, but it would be trivial to always provide the "active" (i.e. the last "before" callback's) id. That is coming in the next PR.

@benjamingr Native Promise support (since that's all we could guarantee) depends on our willingness to either override some native methods in order to collect additional information, or the v8 API adding additional hooks so we can see into the MicrotaskQueue's black box. i.e. will be happy to include support as soon as we have the necessary APIs to do so.

[jasnell]

@trevnorris ... what shape do you expect asyncwrap to be in by the time we're looking to cut v6?

[bnoordhuis]

Can you use the Context-taking, MaybeLocal-returning overload of Get() here?

[trevnorris]

Can do.

[AndreasMadsen]

@trevnorris I'm still not sure I understand the purpose of the final hook. Is it for performance reasons or an alternative to monkey patching nextTick? I will spend some more time on it.

Reason I haven't included it here is because additional API is required to make it useful.

Can you elaborate on what is not included?

but it would be trivial to always provide the "active" (i.e. the last "before" callback's) id. That is coming in the next PR.

That seams dangerous, because one can't distinguish between a defined parent relation (TCP) and a guessed/implicit parent relation (from pre hook). But once timers, nextTick, promised and native addons are properly integrated into AsyncWrap, the implicit parent relation from the pre hook will have a guaranteed correctness.

[trevnorris]

@AndreasMadsen

Is it for performance reasons or an alternative to monkey patching nextTick?

It's mainly an alternative to patching nextTick, but doing this because of performance. Wrapping every nextTick callback is too expensive to make it useful. Meaning every nextTick callback cannot run in its own pre/post phase. After a lot of implementation attempts, my conclusion is that doing so will incur enough of a performance hit to prevent async wrap from being useful.

The goal is simply to allow the parent context to propagate. Which can be achieved by grouping all nextTick callback calls. This is possible because nextTick callbacks effectively run within the same parent context as the callback. Since they don't technically run at a different asynchronous phase as far as the event loop is concerned.

Can you elaborate on what is not included?

If no nextTick callback throws then the current patch mostly works (found a bug that I'll patch tomorrow). But if one does throw then the nextTickQueue halts execution and queues the further processing of callbacks in a setImmediate. When this happens the parent context will have changed when executing the remaining set of nextTick callbacks. So the correct parent id must be preserved and restored when processing those callbacks in the future.

What's not included is the retaining of the parent id when a nextTick callback throws so the remaining queue can properly be processed. I imagined the API to look similar to the following:

const async_wrap = process.binding('async_wrap');
const net = require('net');
const async_map = new Map();

function init(id, provider, parent_id) {
  async_map.set(parent_id, this);
}

// setupHooks, enable, etc.

net.createServer(function(conn) {
  process.nextTick(function throwMe() {
    throw new Error('ah crap');
  });

  process.nextTick(function forTheFuture() {
    // This callback will not run immediately after the above throwMe().
    // Meaning the parent id of this AsyncWrap constructor will not be
    // the same as the connection it's currently queued in. For this
    // reason the parent id must be stored so it can correctly propagate.
    net.connect({port: 8123}, () => {});
  });
});

process.on('uncaughtException', function() { });

As far as processing Promise callback in the MicrotaskQueue, it's impossible for those callbacks to throw. So they do not affect how this will be implemented.

That seams dangerous, because one can't distinguish between a defined parent relation (TCP) and a guessed/implicit parent relation (from pre hook)

The relationship is easy to track. I've already done this before in my initial async listener implementation. Except in the case that 1) the parent id is propagated manually in cases like a new TCP connection; 2) implementation details of timers creating multiple JS handles for a single TimerWrap instance; the active parent id is always that of the active MakeCallback execution scope (this includes nested MakeCallback calls).

It will be easiest for me to PR the implementation with tests as an explanation of how this works. Will do that this week.

[mike-kaufman]

Is there a reason for requiring callers to explicitly disable callbacks, vs implicitly disabling them & restoring them?

[trevnorris]

I'm missing context. Users have to explicitly run async_wrap.enable() before callbacks will be called. This case is to handle:

const async_wrap = require('async_wrap');
async_wrap.setupHooks({ ...callbacks... });
async_wrap.enable();
async_wrap.setupHooks({ ...different callbacks... });

To prevent callbacks from being pulled out from underneath the user while operations may be in flight.

[mike-kaufman]

Right, question is why is it the responsibility of the setupHooks() caller to enable()/disable()? i.e., why can't this be done implicitly by setupHooks()?

Same question follows when the handlers are invoked. We know that if an init() callback triggers an async operation, it risks infinite recursion, so why not implicitly disable async callbacks before invoking init()?

[AndreasMadsen]

It's mainly an alternative to patching nextTick, but doing this because of performance. Wrapping every nextTick callback is too expensive to make it useful.

Thanks, this makes sense. I'm a little worried about how the long–stack-traces are going to look if one uses the final events to infer the parent handle context. But we can try it.

Also a test case for the final hook is missing. There is one for when the callbacks throws, but not for the common case (no-throw).

[mike-kaufman]

Apologies if I'm being slow. Two questions:

1. I'm trying to wrap my head around how the final() callback is intended to work. Is it the case that if a handler receives an init() call for some ID m, then they will also (eventually) see the pre(), post() and final() calls for that same ID m (assuming handlers don't throw, which will fail fast)? If I'm understanding the explanation above, the long-term intention is for the call to final() of m to happen, but it may happen in a different MakeCallback() context?
2. Let's assume I'm trying to implement some "thread-local-storage" analog for continuation scopes. Assume an API of put(key, value) and get(key), and assume values are tiered with each continuation. e.g., if I do a get(key), and key doesn't exist in the current scope, it will traverse through parent scopes until the key is found or we're at the end of the chain of scopes. (let me know if this isn't clear). Now, assume the following user code:

var cs = require('my-continuation-storage-library');

function foo() {
  cs.put('mykey', 'foo');

  process.nextTick( function aBar() {
      cs.put('mykey', 'aBar');
      cs.get('mykey'); // should return 'aBar'
  });

  process.nextTick( function bBar() {
           cs.get('mykey'); // should return 'foo'
  });
}

Now, my question is, with the current set of changes, the nextTick operations of aBar() and bBar() will share the same async-scope ID n. Specifically, they will be invoked after a post(n). If I am implementing such a continuation-storage library, how do I distinguish between the two invocations of aBar() and bBar()? From my point-of-view as a user/implementer of such a library, these should be independent invocations, but I don't see how I can differentiate.

Sorry if these are long-winded. :) Let me know if anything is unclear and I'll do my best to clarify.

Thanks,

Mike

[trevnorris]

@AndreasMadsen

Also a test case for the final hook is missing. There is one for when the callbacks throws, but not for the common case (no-throw).

Ah thanks. Missed that.

@mike-kaufman

Apologies if I'm being slow.

No worries. Head wrapping around complex asynchronous call chains and their associated edge cases is mind warping. I've learned this from hundreds of hours spent on the AsyncWrap, and all its previous incarnations.

Is it the case that if a handler receives an init() call for some ID m, then they will also (eventually) see the pre(), post() and final() calls for that same ID m

init only alerts the user that a class has been instantiated that has the capacity to call pre/post/final, but cannot guarantee that it will. e.g.:

net.createServer().listen().close()

will fire init since the server is instantiated, but because no connection is ever received MakeCallback is never fired. Hence pre/post/final can never be triggered.

(assuming handlers don't throw, which will fail fast)

Do you mean one of the async wrap callbacks, or the callback of the object handle passed to MakeCallback? The former will fail fast now (no more abort after this PR). The latter can still be caught using 'uncaughtException' or a domain. In that case execution is allowed to proceed.

the long-term intention is for the call to final() of m to happen, but it may happen in a different MakeCallback() context?

Yes. As I mention above it's possible for execution to proceed even in the case of an exception. Which may execute callbacks in the nextTickQueue in a different parent scope since it will not be drained immediately in the case of an exception.

Now, this does have a flaw that I'm still working out. That is, determining when the nextTickQueue has started processing depends on post firing. But if execution of those callbacks happens in the future then post would fire with a different id. This is a gap, and I'm still figuring out the best API to handle it.

with the current set of changes, the nextTick operations of aBar() and bBar() will share the same async-scope ID n.

Yes.

If I am implementing such a continuation-storage library, how do I distinguish between the two invocations of aBar() and bBar()?

You can't. While it's cheap to store the parent on each nextTick object, running an async wrap pre/post for each callback so expensive that the API becomes near unusable. Further justification comes from the fact that the nextTickQueue technically runs (minus any exceptions) in the same execution scope of the MakeCallback(), and AsyncWrap is more concerned with the low level asynchronous events.

At the moment I'm considering backing out the final callback and moving post to the end of MakeCallback. Am going to investigate that implementation. It comes with a slightly different paradigm, so will probably need to do more explaining later.

From my point-of-view as a user/implementer of such a library, these should be independent invocations, but I don't see how I can differentiate.

To reiterate, it's prohibitively expensive to do this. Enabling async wrap with init/pre/post/final/destroy and running benchmarks/http_simple.js the sum of execution of all those callbacks is over 200k/sec. Those callbacks were designed to only address major events in the async lifecycle of a handle/request, and it's still generating a tremendous amount of data.

That explain everything?

[mike-kaufman]

it's prohibitively expensive to do this. Enabling async wrap with init / pre / post / final / destroy and running benchmarks/http_simple.js the sum of execution of all those callbacks is over 200k/sec.

Sorry, I'm not following the 200k/sec. Do you mean the benchmark executes over 200k callbacks/second? Do you know how this impacts benchmark measurements?

[trevnorris]

@mike-kaufman

Do you mean the benchmark executes over 200k callbacks/second?

The sum of calls made to init/pre/post/final/destroy are in excess of 200k/second.

Do you know how this impacts benchmark measurements?

I added the simple operation of tracking every handle in a Map by adding it in init and removing it in destroy. Doing that performance impact was < 8%.

[AndreasMadsen]

At the moment I'm considering backing out the final callback and moving post to the end of MakeCallback.

@trevnorris Could we back out both the final hook and the the post hook change you are proposing here. It is quite complex and I fell it is better discussed in a standalone PR. The rest of this PR is fairly minor stuff that is much easier to approve.

[trevnorris]

@AndreasMadsen Can do. I'll make a separate pr for the post changes.