feat(context): implement withAsync #752

[vmarchaud]

I took @dyladan initial work from his branch and tried to debug the following test case which wasnt working:

Note: i will refer to scope when speaking about javascript context (this) and context when refering about our implementation.

it('should finally restore an old scope', async () => {
  const scope1 = '1' as any;
  const scope2 = '2' as any;
  let done = false;

  await contextManager.withAsync(scope1, async () => {
    assert.strictEqual(contextManager.active(), scope1);
    await contextManager.withAsync(scope2, async () => {
      assert.strictEqual(contextManager.active(), scope2);
      done = true;
    });
	// HERE
    assert.strictEqual(contextManager.active(), scope1);
  });

  assert.ok(done);
});

The things to know when working with async/await is that V8 is essentially wraping the code that follow the await inside a new scope. The async_hooks API allows us to track the creation of these new scopes so we can associate the newly created scopes ids with the context available at the time of their creation.

So in the test case, where i placed the // HERE comment, we are in a new scope that should contains the correct context because as said above we associated scope and their corresponding context. The problem is that the context is associated with a given scope change. In our case:

await contextManager.withAsync(scope1, async () => {
  assert.strictEqual(contextManager.active(), scope1);
  // scope is 42, context is 1
  await contextManager.withAsync(scope2, async () => {
    // scope is 43, context is 2
    assert.strictEqual(contextManager.active(), scope2);
    done = true;
    // At this time, v8 create a new scope (44) which the code after the resolve of the promise
   // we get notified with async hooks and set its context to 2 because that the current context
  });
  // after the callback is "awaited", we re-apply the old context by setting scope 42 to 1
  // however as v8 changed the scope we are effectively in the scope 44 and its context is 2
  assert.strictEqual(contextManager.active(), scope1);
});

The fix is pretty simple in theory, which map a given javascript context to a pointer that hold a reference to a context.
Since the only reference that exists in javascript are object, i added a simple Reference class which is only used to hold a reference to the context.

I'm pretty sure that my explaination is not clear so feel free to ask questions if you have any.

Also, i will make a separate PR for adding withAsync on the tracers. I will believe this one is already quite hard to review to not add more code !

[codecov-io]

Codecov Report

Merging #926 into master will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #926      +/-   ##
==========================================
+ Coverage   94.95%   95.03%   +0.07%     
==========================================
  Files         210      210              
  Lines        8571     8705     +134     
  Branches      772      776       +4     
==========================================
+ Hits         8139     8273     +134     
  Misses        432      432              

Impacted Files	Coverage Δ
...ontext-async-hooks/src/AsyncHooksContextManager.ts	97.41% <100.00%> (+0.82%)	⬆️
...-async-hooks/test/AsyncHooksContextManager.test.ts	100.00% <100.00%> (ø)

[Flarna]

As usual it's hard to follow AsyncHooks, in special in combination with async as several promises are created for each async/await.

Maybe we should add some more tests with mixed of sync/async callbacks?
Maybe also verify how context is passed if e.g. the result of withAsync is not awaited?
Or if a (not promisified) async operation is triggered within a callback (e.g. setTimeout)?

This is for sure not blocking and can be done in separate PRs. But in general I have the feeling more scenarios in the tests helps to understand who it works even if the test coverage doesn't increase.

[vmarchaud]

@Flarna Totally agree, i believe its in the scope of that PR so i will add few more cases

[vmarchaud]

I've added some, the only counter intuitive one would be:

it('should correctly restore scope', done => {
  const scope1 = '1' as any;
  const scope2 = '2' as any;

  contextManager.with(scope1, () => {
    assert.strictEqual(contextManager.active(), scope1);
    contextManager
      .withAsync(scope2, async () => {
        assert.strictEqual(contextManager.active(), scope2);
      })
      .then(() => {
        assert.strictEqual(contextManager.active(), scope1);
        return done();
      });
    // in this case the current scope is 2 since we
    // didnt waited the withAsync call
    // however some would expect that the scope should be 1
    assert.strictEqual(contextManager.active(), scope2);
  });
});

I would argue that even though its counter intuitive, its working as intented. One more reason for developers to avoid floating promises in their code.

[vmarchaud]

@Flarna Addressed your comments once again !

[vmarchaud]

@open-telemetry/javascript-approvers Need some reviews here !

[obecny]

lgtm, however I would suggest to combine the body of those 2 functions

[vmarchaud]

On the performance side, i've tested with on master which gave those results:

#with x 153,564 ops/sec ±1.39% (500 runs sampled)

and with this branch (in its current form):

#with      x 160,675 ops/sec ±0.79% (500 runs sampled)
#withAsync x  26,827 ops/sec ±3.46% (500 runs sampled)

with has actually better performance which might come from V8 having a better optimization path when using Map instead of raw object.
withAsync is without surprise a lot slower since using promises, i don't think we can do much more on our side to optimise that.

[vmarchaud]

I've decided to remove withAsync from the context manager interface for know since i believe it's not possible to implement it on the browser side. I think we should merge as-is and expose it in on the node tracer on a separate PR, WDYT ?

[dyladan]

I think it's a good idea to only surface it as part of the SDK not the API. That way, we're not locking ourselves into supporting it everywhere when that may not be possible.

Do we also want to consider marking it as experimental in some way? Not sure if there is some tsdoc @flag we can use or something like that

[vmarchaud]

@dyladan I would prefer to mark as experimental the withAsync on the tracer instead of the context manager since it's mostly likely the only way user will use it.

[vmarchaud]

@mayurkale22 I think we could include this in the 0.7 release too if there are no blockers on your side ?

If you don't have any blocker on your side could we merge this one after the release so i can make the next PR to add withAsync on the tracer ?

[SimenB]

Just a drive by comment in case it's useful - Node natively includes experimental support for context between async calls. I'm unable to link directly, but its under the "Experimental Async Local Storage API" heading here: https://medium.com/@nodejs/node-js-version-14-available-now-8170d384567e

I'll paste in the paragraph below

The project has been working on APIs to help manage context across Asynchronous Calls over a number of releases. The experimental Async Hooks API was introduced in earlier versions as part of this work. One of the key use cases for Async Hooks was Async Local Storage (also referred to as Continuation Local Storage). There have been a number of npm modules that have provided APIs to address this need, however, over the years these have been tricky to maintain outside of Node.js core and the project reached a consensus that exploring having Node.js provide an API would make sense. The 14.x release brings an experimental Async Local storage API (which was also backported into 13.10) https://nodejs.org/api/async_hooks.html#async_hooks_class_asynclocalstorage. We are looking for the community to try out this API and give us feedback on abstraction model, API interface, use case coverage, functional stability, naming, documentation etc. so that we can work on getting it out of experimental in later releases. The best way to provide feedback is to open an issue in the diagnostics repository here (https://github.com/nodejs/diagnostics/issues) with a title along the lines of “Experience report with AsyncLocalStorage API”.

I'm thinking this project is the perfect one to "try out this API and give us feedback" 😀 You might already know about this of course, but I tried searching this repo for mentions without finding it, so thought I'd drop a comment at least