The case for _not_ binding around awaits

[Qard]

I had a call with @andreubotella the other day explaining some of what APM vendors want from context management that we don't have now, explaining the use cases, how we are limited presently, and how some aspects of the proposal at present somewhat limit us too. One of the main issues discussed was that capturing context when an await happens and then restoring at that point cuts off a branch of the graph which we are interested in.

Some examples:

function readFile(path) {
  // Imagine there's something here to create and store a span for this call scope...

  // We want to produce another span by instrumenting within the openFile function.
  // This span should have a child-of (OpenTelemetry concept) relationship to
  // the readFile span. This is fine as the context will flow into the call.
  const fd = await openFile(path)

  // We want readFd to have a follows-from (OpenTelemetry concept) relationship
  // to the openFile span so we need for that span to be the context value here.
  // This is a problem if we snapshot the context at the await and restore after as
  // the context value would be the readFile span instead of the openFile span.
  const data = await readFd(fd)

  // As with readFd, close should receive the context from the path through readFd,
  // meaning it should have a follows-from relationship to the readFd span.
  await closeFd(fd)

  return data
}

In this scenario we want the execution branches which merge back through an await to also merge back their context. If we instead just relied on context flowing through the promise resolve we would get this automatically and, if no context adjustment was made within openFile, we would still get the same flow expected by binding at awaits.

Another real-world scenario is that we instrument aws-sdk to capture SQS jobs being processed and linking them into a distributed trace. A job processor would look something like this:

const { SQSClient, ReceiveMessageCommand } = require("@aws-sdk/client-sqs")

const client = new SQSClient(clientConfig)
const command = new ReceiveMessageCommand(commandConfig)

async function processor() {
  while (true) {
    // The job received here has distributed tracing headers passed from another service.
    // We want to create a span when client.send(...) receives the result of the command
    // and finds distributed tracing headers. This span should then be stored in the context
    // so subsequent execution can connect to that span.
    const command = new ReceiveMessageCommand(input)
    const response = await client.send(command)

    // Do some processing...
  }
}

In the above example we want each await client.send(...) to flow the span created within out to the async function it's being called within. Subsequent execution within that function until the next await client.send(...) should be considered as continuations of that context. Because of promises flowing context through the resolve path, if this were to be rewritten with promise.then(...) nesting instead of async/await this would behave as expected, which leads me to believe that binding around awaits is actually incorrect. What we should be doing is just letting the context flow into the functions being called and merged back out of whatever the resolution of the awaited promises are. Context is meant to follow execution behaviour, so if execution is merging back from elsewhere so too should context.

If we have a way to access the calling context rather than the bound context then this would be a non-issue, but if we're aiming for a single blessed path then this would make the current design of context flow completely unusable for observability products.

As it is presently, context within an async function is effectively no different from local variables. The stack restores in the same way as the context binding over an await does, so they are functionally the same at that point. The whole point of adding context management to the language is that it can flow values through paths which existing code can not.

[andreubotella]

Because of promises flowing context through the resolve path, if this were to be rewritten with promise.then(...) nesting instead of async/await this would behave as expected, which leads me to believe that binding around awaits is actually incorrect.

That is not correct: in the spec text as it currently stands, calling promise.then() takes a snapshot which is stored in the promise reaction job, and which gets restored whenever that job gets invoked after the promise resolves. This is exactly the same behavior as CPED in V8 has currently.

[Qard]

Ah...well then that is wrong too. 😅

My understanding from prior conversations here was that we had went for following resolve path rather than promise construction path. We want context to flow from the point a promise is resolved to the point where a continuation from that resolution would run.

[Flarna]

Do you want readFd to be only a follows-from of openFile or additionally a child-of readFile?
Similar closeFd could be additionally a child-of readFile.

If both relationships should be kept it's not about choosing a path - both are needed.
And potentially even more if one has two similar relationships already inside readFile because of inner readFile operations.

[mhofman]

This is starting to touch on the problem I tried to convey last year: a promise reaction is mainly a join of 2 paths in the execution graph: the resolving path and the subscribing path. This is actually something most telemetry tools I've seen don't have a way to represent (otel supports links, but they are rarely/never displayed this way by otel tools which represent the execution mostly as a tree).

This is particularly apparent with Promise.all which is a join of all the promises resolving paths with a single subscription path.

One could argue there is a 3rd context: the promise creation time, but that is just the point where the execution graph "forked", and as such is usually recorded by any "call" instrumentation. It is also most of the time the same as the registration context.

Both resolving and subscription contexts are necessary for building distributed debuggers allowing to visualize the execution graph. One such example is Causeway.

[Qard]

So this "third" point you mention is more the difference between the scope of the store.run(...) and the point within that where an async task is scheduled. Any async task scheduled directly within the sync execution which occurs within the function given to store.run(...) should capture the context at the point the scheduling occurs. In the case of a promise, the construction of the promise doesn't schedule anything, it's the resolution of the promise which schedules a microtask.

With context management we basically always want to capture where scheduling happens and resume where continuation happens. In a callback-receiving function this is fairly straightforward that we bind the callback to return to the context captured from the original call. It's a bit fuzzier with promises and especially async/await, but we generally want the resolution of a promise to flow the context at that point to the continuation which runs from that resolution. It should flow in that way both with promise.then(...) and also around await promise. The context at promise construction should flow into the code which manages that construction and eventually leads to resolution, but it should not be capturing at that point as that will be skipping over a bunch of internal behaviour we want to be able to flow through.

If we have a way to escape the bound context back to that scheduling path then it doesn't matter if it follows this path, but we do need the tools to get to that path.

[jridgewell]

I want to build an example from your OP code:

const ctx = new AsyncContext.Variable();
function readFile(path) {
  const span = new Span();
  return ctx.run(span, () => {
    const fd = await openFile(path);

    // This is the core ask, that awaiting can change the context.
    assert.notEqual(ctx.get(), span);

    // …
    return data;
  });
}

But this is essentially the same as mutating a context in a child call and expecting it to be present in the parent execution:

const ctx = new AsyncContext.Variable();
function readFile(path) {
  const span = new Span();
  return ctx.run(span, () => {
    const open = openFile(path);

    // Calling `openFile` was allowed to change my context.
    assert.notEqual(ctx.get(), span);

    const fd = await open;

    // …
    return data;
  });
}

I think making this change is a mistake. Rather than allowing openFile to replace the current value stored in ctx, it should mutate the object's properties.

As it is presently, context within an async function is effectively no different from local variables. The stack restores in the same way as the context binding over an await does, so they are functionally the same at that point. The whole point of adding context management to the language is that it can flow values through paths which existing code can not.

This was an important design goal so that we didn't violate dynamic scoping. The same as the discussion we're having in tc39/proposal-using-enforcement#1, I think allowing a child call to replace the context is a mistake.

My understanding from prior conversations here was that we had went for following resolve path rather than promise construction path. We want context to flow from the point a promise is resolved to the point where a continuation from that resolution would run.

reject behaves this way specifically for the execution of unhandledrejection, but it's completely unobservable otherwise. If you promise.then(cb) or promise.catch(cb), the cb will always execute within the context that is active during the promise.then() call.

Under all circumstances, promise.then(()=>console.log(ctx.get())) and await promise; console.log(ctx.get()) must be behave the same.

[andreubotella]

But this is essentially the same as mutating a context in a child call and expecting it to be present in the parent execution:

const ctx = new AsyncContext.Variable();
function readFile(path) {
  const span = new Span();
  return ctx.run(span, () => {
    const open = openFile(path);

    // Calling `openFile` was allowed to change my context.
    assert.notEqual(ctx.get(), span);

    const fd = await open;

    // …
    return data;
  });
}

My understanding of @Qard's proposal is that ctx.get() would not change before the await – instead await open would cause the mapping after the await to be set to the mapping in which open was resolved. The same would go for open.then(() => ctx.get()).

This would not be the function's context changing arbitrarily, it would only happen at await points in async functions. In a sense this would be an equivalent of not restoring the context across yield points in generators.

[Jamesernator]

But this is essentially the same as mutating a context in a child call and expecting it to be present in the parent execution:

I think making this change is a mistake.

It's not just a mistake, it completely breaks the feature for anything other than tracing. For scheduling outer task will suddenly start gaining the priority of tasks that were scheduled (even arbitrarily deeply) inside other tasks, for things like dependency injection this would mean inner injections would bleed out to the upper context.

In order to repair this, you would have to wrap every await with a thing that restores the intended context, i.e. for scheduling you'd have to do:

await scheduler.runSubTask(...);

on every await everywhere. This is exactly the thing AsyncContext was meant to avoid.

---

I don't see that any solution for the tracing use case other than just providing AsyncContext.callingContext(), although as @mhofman points out, with Promise.all a single calling context doesn't make sense, probably just returning an array of contexts is enough, i.e. AsyncContext.callingContexts().

Something I am wary though of the fact that any form of AsyncContext.callingContexts() or similar significantly extends the lifetime of any snapshotted variables even when those variables may wish to be cleared as early as possible.

Like for termination tracking, termination can't happen until the parent context ends as well:

const task = await scheduler.measureTask(theTask, (totalTime) => {
    console.log(`Took: `, totalTime);
});

// This following code is *still* within the measurement lifetime because
// AsyncContext.callingContexts() could possibly be called
for (let i = 0; i < 100000000; i+=1) {
   doHeavyComputation();
}

[Jamesernator]

Perhaps there should be second kind of variable (or option to AsyncContext.Variable) specifically designed for tracing-like use cases. (Regular variables would not hold onto their calling context). i.e.:

const tracingVariable = new AsyncContext.TracingVariable();

tracingVariable.run("call", async function() {
    console.log(tracingVariable.get()); // "call"
    console.log(tracingVariable.callingContexts()); // []
    await Promise.all([
        new Promise((resolve) => {
            tracingVariable.run("promise-1", () => {
                resolve();
            });
        }),
        new Promise((reject) => {
            tracingVariable.run("promise-2", () => {
                resolve();
            });
        }),
    ]);
    console.log(tracingVariable.get()); // "call"
    console.log(tracingVariable.callingContexts()); // ["promise-1", "promise-2"]
});

[Qard]

My understanding of @Qard's proposal is that ctx.get() would not change before the await – instead await open would cause the mapping after the await to be set to the mapping in which open was resolved. The same would go for open.then(() => ctx.get()).

Yes, this is what I mean. Context should definitely not leak out of a context scope, but it should flow through a continuation path, which means that if a promise resolves with a different context then that different context should be what comes out on the other side of the await. If no context change happened before it reached the resolve than it would use the same context as before without needing to bind around the await, it would just gain the additional capability of flowing context values through continuation paths.

Context is meant to model execution flow, so when execution flow is merging back then the context flow should merge back too.

[Qard]

In order to repair this, you would have to wrap every await with a thing that restores the intended context, i.e. for scheduling you'd have to do:

await scheduler.runSubTask(...);

This is exactly what should be happening. If you want to orphan a sub-graph by binding around it then it probably should be explicit.

I think generally speaking though that context is meant to flow to logically continuing execution. In callback-based code a call which schedules an async task would capture a snapshot and restore that snapshot around the execution of the callback. The exact same behaviour should be expected of a promise, both with p.then(...) chaining and with async/await.

It's important that the innermost path be followed and that parts of the graph do not get stepped over when propagating the context. It's acceptable for context values to flow to a point and then have a bind occur to swap that value out, but that context should flow to that point and should be primarily propagating around the points where tasks are scheduled not where continuations like callbacks are received as the receipt point often has further things within which need context for tracing, but also there could be multiple points at which tasks could be "scheduled" for a thing which is a single operation from the perspective of making a call, so each scheduling should be capturing the context at its appropriate point to be correct.

[Jamesernator]

This is exactly what should be happening.

No, this is completely unusable for all the non-tracing use cases, the whole point of AsyncContext is that users don't want to thread things through many layers of functions. If you have to wrap anyway there is little value in this proposal for any of the other use cases.

It's important that the innermost path be followed and that parts of the graph do not get stepped over when propagating the context

No, it's only important for the tracing use case, for all other use cases this is an anti-feature.

[Jamesernator]

Just to give an example of why the proposed behaviour is completely unusable, consider for example the dependency injection use case:

// In practice the logger would be a wrapper around this, but this is fine for the example
const logger = new AsyncContext.Variable({ default: consoleLogger });

async function subTask() {
    // At the end of the subtask
    return await logger.run(fileLogger("subTask.log", async function() {
        logger.get().print("In the subtask");
    });
}

await logger.run(fileLogger("main.log"), async function() {
   logger.get().print("Starting the subtask");
   await subTask();
   logger.get().print("Ending the subtask");
});

In this example, your proposed behaviour would have "Ending the subtask" be printed to the subTask.log file, that is absolutely not the behaviour one would want for dependency injection use cases.

[Qard]

the whole point of AsyncContext is that users don't want to thread things through many layers of functions

Yes, I am in agreement with this, and I do think that the ideal state is that context has a most reasonable default flow which avoids needing these manual binds. But there is no path which is universal, even excluding the tracing cases. We need to have mechanisms of getting the context values from other execution paths. A default can and should be chosen in the general case, but we need flexibility or the feature will be useless even for the basic application developer use case as there will be many cases where it is needed to provide some manual signals about path preference.

No, it's only important for the tracing use case, for all other use cases this is an anti-feature.

This is absolutely false. Flowing through the inner paths are required for correct functioning or users will most certainly try to access context in internal flows of complex interactions they are creating and have it not work the way they expected because we didn't flow the context through all execution. For example, say you have a database client connection object and make a query call which takes a callback, but as part of the connection you have some session management step that can be defined as another function, or perhaps an event--if you don't flow through the internal path to where that session management step is executed and instead step over from call to callback the user could try to read the context expecting a value to be present because it logically flows from the call and be frustrated and confused when they instead get an empty context.

Context can not function correctly unless the innermost path is followed. We (APMs) have done extensive research on this subject to prove that this is the case and I have a whole bunch of documentation on this subject which I will be sharing soon when I have had the time to clean it up.

In this example, your proposed behaviour would have "Ending the subtask" be printed to the subTask.log file, that is absolutely not the behaviour one would want for dependency injection use cases.

That actually usually has been the expected behaviour with AsyncLocalStorage in Node.js. It currently doesn't function this way (due to limitations of the implementation) and we have received many bug reports over the years from people expecting for context to flow to logically continuing code. Certainly in this specific example it can be a bit more ambiguous which flow is correct, and this is why I keep pushing that we need control of context flow. I will repeat this as many times as is necessary for people to finally acknowledge it: There is no single perfect context flow, we need per-store context flowing capabilities, and in the most user-friendly way possible.

[Jamesernator]

A default can and should be chosen in the general case, but we need flexibility or the feature will be useless even for the basic application developer use case as there will be many cases where it is needed to provide some manual signals about path preference

I agree that there should be a way to get other contexts, but I completely believe that the current design is the only coherent behaviour for await.

There is no way to keep the registration, i.e. the await context, without boilerplate on every single await. However the calling context can be accomplished merely by providing a function AsyncContext.callingContexts().

(Users are typically not going to the ones calling these functions, so the fact AsyncContext.callingContexts() adds boilerplate isn't really that important, that'll be hidden behind methods like .trace() or such).

That actually usually has been the expected behaviour with AsyncLocalStorage in Node.js. It currently doesn't function this way (due to limitations of the implementation) and we have received many bug reports over the years from people expecting for context to flow to logically continuing code.

Developers often want magic that isn't coherent beyond a narrow particular usage. In particular we return to the fact that there is arbitrarily many possible contexts when Promise.all is used.

Also from looking at the issues on AsyncLocalStorage, many of them seem to be about .enterWith, which is unsurprising that people would expect this flow upwards as there is no corresponding .exit.

For example, say you have a database client connection object and make a query call which takes a callback, but as part of the connection you have some session management step that can be defined as another function, or perhaps an event--if you don't flow through the internal path to where that session management step is executed and instead step over from call to callback the user could try to read the context expecting a value to be present because it logically flows from the call and be frustrated and confused when they instead get an empty context.

This is yet another case that needs to be made coherent under the existence of many contexts (i.e. Promise.all again). If the database and connection are cooperating then they can look through all relevant contexts with AsyncContext.callingContexts() (or similar) and the current context and pick whichever one is most relevant.