00:30
<littledan>
I had some quick chats today at TC39 with dminor and bakkot, everyone seemed cautiously positive though a little uncertain about use cases. I am optimistic about the presentation but we might consider adding bonus slides that go in more depth there (eg just taking what was in legendecas’s webperf wg slides)
00:31
<littledan>
I didn’t get the feeling that the complexity around what things are wrapped was considered a very bad issue for either (just something to work out) but maybe I misunderstood them
00:32
<littledan>
Good luck this week on proposing for Stage 2!
15:16
<James M Snell>

So, here's a question. fetch has the Response object. This can be created using a ReadableStream object, which uses a number of internal promises to manage state. In workerd, we have a method that handles received requests and can return a Response object specifying the response to an http request. So, imagine a case like:

const als = new AsyncLocalStorage(); // or AsyncContext
export default {
  async fetch(req) {
    const readable = als.run('abc', () => new ReadableStream({
      pull(c) {
        c.error(new Error(`boom ${als.getStore()}`));
      }
    }));
    return als.run(123, () => new Response(readable));
  }
}

The actual pipe from the readable happens by the code that calls the fetch function handler here, which is running in the root async context where als store will be undefined.

What value would you expect als.getStore() to return when the error is constructed?

15:17
<James M Snell>
Keep in mind that the pull function can be called at different times depending on the value of the highWaterMark configured for the readablestream
15:23
<James M Snell>
specifically, pull might be called when the ReadableStream is created, and any time after while it is being read
15:52
<Justin Ridgewell>
I don't have a good answer. Just from the use case I recognize that it must be one of the two (and not undefined just because the Response object is leaked out of the fetch), but I don't have a good idea of which one of these contexts is the "registration" context
15:53
<Justin Ridgewell>
In my experience, the contexts are the same. I create the readable and the response in the same overall context, so the distinction doesn't matter
15:56
<James M Snell>
That's fair. What I suspect is that for web apis, once AsyncContext is integrated, we'll need a way of identifying in webidl which types are expected to capture and propagate the async context
15:56
<James M Snell>
It's going to be needed regardless of where AsyncContext ultimately gets defined (tc39 or whatwg, etc)
17:51
<Ben Newman (Apollo, @benjamn on GH)>
Here’s my take: if/once AsyncContext becomes an official part of the language, utilities like ReadableStream could (backwards compatibly!) begin to support a contract whereby the context in effect at ReadableStream construction time is automatically restored each time pull and other callbacks are called
17:51
<Ben Newman (Apollo, @benjamn on GH)>
In the meantime (potentially for a long time), you might need to take matters into your own hands using AsyncContext.wrap:
17:51
<Ben Newman (Apollo, @benjamn on GH)>
  async fetch(req) {
    const readable = als.run('abc', () => new ReadableStream({
      pull: AsyncContext.wrap((c) => {
        c.error(new Error(`boom ${als.getStore()}`));
      }),
    }));
    return als.run(123, () => new Response(readable));
  }
}
17:51
<Ben Newman (Apollo, @benjamn on GH)>
With this code, the als with abc is bound to the pull callback function so als.getStore() can return abc instead of undefined for the Error
17:52
<Ben Newman (Apollo, @benjamn on GH)>
Of course, this AsyncContext.wrap binding means any other AsyncContext values coming from the caller of the pull callback (some code in the ReadableStream implementation) will now be ignored, but that’s probably safe as long as you don’t expect ReadableStream to be manipulating AsyncContext
17:52
<Ben Newman (Apollo, @benjamn on GH)>
Is that helpful?
17:55
<James M Snell>
Unfortunately it doesn't exactly help. If I call c.error() within the pull, and there's a queued read, that queued read is likely not necessarily going to be rejected immediately when the c.error is called. It might end up getting processed from a different async scope, which means if it results in an unhandledrejection event, it will propagate the wrong context
17:56
<James M Snell>
going to have to play with this case a bit more to really tease out all the various cases
17:57
<James M Snell>
There are a variety of situations that make this complicated.
17:57
<Ben Newman (Apollo, @benjamn on GH)>
whatever code is scheduling/queueing those read jobs needs to be using something like AsyncContext.wrap to preserve the context from one job to the next, I think?
17:58
<Ben Newman (Apollo, @benjamn on GH)>
are you trying to get this to work without modifying library code that does this kind of scheduling? (seems hard)
17:58
<James M Snell>
for instance, I may have multiple reads queued, all called from separate async contexts, but a mismatched number of pulls, where one pull triggered by one context fulfills multiple reads
17:58
<James M Snell>
yeah, we're not in control of the ReadableStream implementations
17:58
<Ben Newman (Apollo, @benjamn on GH)>
ok I think I see the rub you were pointing out
18:00
<Ben Newman (Apollo, @benjamn on GH)>
a single invocation of pull can handle/inherit multiple different contextual branches
18:02
<Ben Newman (Apollo, @benjamn on GH)>
do you think the solution is somehow to merge the branches into one context, or to keep the different branches distinct so pull can create the errors with their correct (not merged) contexts?
18:03
<littledan>
That's fair. What I suspect is that for web apis, once AsyncContext is integrated, we'll need a way of identifying in webidl which types are expected to capture and propagate the async context
This differs from how Yoav ended up implementing a similar feature in Blink. It turned out that the WebIDL layer approach was too slow.
18:03
<littledan>
instead, it's more like, you propagate the context when queueing a task
18:09
<James M Snell>
so in this case it's a bit complicated. "when queing a task" ... for a readable stream, a pending read can be viewed as a queued task
18:09
<James M Snell>
when a read is received, the stream may or may not call pull to ensure that the read can be fulfilled
18:09
<James M Snell>
and a single pull can cause multiple pending reads to be fulfilled
18:10
<James M Snell>
so, within that pull, should it use the context of the read that immediately triggered it?
18:10
<James M Snell>
or should it use the context that was current when the stream was created?
18:11
<James M Snell>
and if that pull causes an error, causing all subsequent reads to reject with unhandled rejections, should the unhandled rejection events be called from the async context for the individual read or the context that was active when the pull errored
18:13
<Ben Newman (Apollo, @benjamn on GH)>
is there some mechanism currently for the pull callback to tell why an error occurred, in a way that points back to one particular read?
18:13
<James M Snell>
no
18:14
<James M Snell>
whether it throws or calls controller.error(...) the effect is the same, the stream is put into an errored state such that all pending and future reads are rejected with that reason
18:14
<James M Snell>
all pending reads would be rejected within the same async context and not the frame that was current when the read was scheduled
18:15
<James M Snell>
so the unhandledrejection handler would see that context and the context that scheduled the read in the first place would be lost
18:17
<James M Snell>

Here's another example:

const als = new AsyncLocalStorage();

const readable = als.run(123, () => new ReadableStream({
  pull(c) {
    c.error(`boom ${als.getStore()}`);
  }
}, { highWaterMark: 0 }));
const response = als.run('abc', () => new Response(readable));
const reader = als.run('xyz', () => response.body);
const read1 = als.run(321, () => reader.read());
const read2 = als.run(567, () => reader.read());
await Promise.all([
  als.run('???', async () => await read1),
  als.run('!!!', async () => await read2)
]);
18:18
<James M Snell>
sorry that's a bit off... one sec
18:18
<Ben Newman (Apollo, @benjamn on GH)>
I'm not sure this works as you'd hope: const reader = als.run('xyz', () => response.body), since reader.read won't automatically restore the xyz context value when called
18:19
<James M Snell>
that's thrown in just to make it more complex. I know it wouldn't do anything :-)
18:23
<Ben Newman (Apollo, @benjamn on GH)>
would it be acceptable for the unhandledrejection error to report a set of contexts rather than just one? or is that a nonstarter?
18:24
<James M Snell>
it's called once per each unhandled promise rejection, there's only one context associated with each
18:24
<Mathieu Hofman>
I have been thinking we need to extend AsyncContext itself to handle a set of context
18:24
<James M Snell>
reporting a set would not make sense there
18:25
<Ben Newman (Apollo, @benjamn on GH)>
how does pull know which read was to blame, and so which context to report?
18:26
<Mathieu Hofman>
so that the context user can gather the values associated with both the registration and the trigger paths
18:26
<James M Snell>
it doesn't. Currently pull would be called within the async context of whatever read triggered it. It could also be called following the completion of the start algorithm microtask (the async context where the stream is constructed).
18:28
<James M Snell>

we can break this down to a few specific questions:

  1. Because the various "algorithms" associated with a stream are considered part of its internal implementation (e.g. start, pull, close), should those always be executed in the async context frame in which the stream itself was created or should they propagate the context that triggered the algorithm to be called
18:29
<James M Snell>
  1. If a read promise rejects and is unhandled, should the unhandledrejection event be called in the async context in which the read promise was rejected (which is likely different than the context that scheduled the read), or the async context that was current when the read was scheduled
18:34
<Ben Newman (Apollo, @benjamn on GH)>
Mark Miller brought up a point a while back (when AsyncContext was last proposed), about what to do with an AsyncContext.wrap-bound function that gets called in a context where some AsyncContext instance that was previously bound with one value (for some AsyncContext) happens to have a different value in the new calling context
18:34
<James M Snell>

Keeping in mind also that people can do silly things like..

let controller;
const readable = als.run(123, () => new ReadableStream({
  start(c) { controller = c; }
}));
const read = als.run('abc', () => readable.getReader().read());
als.run(321, () => controller.error('boom'));
18:36
<Ben Newman (Apollo, @benjamn on GH)>
Are you expecting the context to be propagated somehow by the assignment controller = c? Whatever context is in effect in start is pretty clearly lost by this code, right?
18:36
<Ben Newman (Apollo, @benjamn on GH)>
I don't expect controller.error to have access to anything beyond the 321 context, there
18:37
<James M Snell>
nope, it's definitely lost. so if we always called the algorithm in the context captured when the readablestream was created, this would escape that context and would cause the reads to be rejected in the context where the value is 321
18:37
<James M Snell>
but what should be the context for the unhandledrejection associated with the read
18:37
<James M Snell>
'abc' or '321'?
18:39
<Ben Newman (Apollo, @benjamn on GH)>
I can imagine a Promise implementation that has a chance of preserving abc (in this code) in the final context associated with the rejected read promise
18:41
<Ben Newman (Apollo, @benjamn on GH)>
but that's assuming/imagining Promise objects capture context at creation time (when readable.getReader().read() is called), which is yet to be decided or implemented
18:42
<Mathieu Hofman>
Why would a promise ever need to capture the creation time? I only see the resolution and registration times as being useful
18:43
<Ben Newman (Apollo, @benjamn on GH)>
for unhandledrejection error attribution/context, mostly, I think?
18:43
<James M Snell>
we've previously decided that, for promises, the context would be captured at the moment the continuation was attached. So either when then is called or we await
18:43
<Mathieu Hofman>
that's the rejection time that's useful, not the promise creation time
18:43
<James M Snell>
and that in the deferred promise case, we'd propagate the context that is current when either resolve() or reject() is called
18:44
<Ben Newman (Apollo, @benjamn on GH)>
I'm fine with that
18:44
<James M Snell>
but in the ReadableStream case, it's most useful to treat read() itself as a scheduled task
18:44
<Ben Newman (Apollo, @benjamn on GH)>
so it's the context when reject() is called that matters (determines the context reported by the unhandledrejection error)?
18:44
<James M Snell>
since the read promise itself might be rejected from a completely unrelated context
18:46
<Mathieu Hofman>
wouldn't the onus be on the read() implementation to reject the promise from the context in which read() was called if the rejection is related to the read implementation.
18:47
<James M Snell>
yeah, it's not specific to the promise itself
18:47
<James M Snell>
should the read operation itself capture the current context and make sure that whenever that queued promise is resolved or rejected the captured context is entered
18:48
<James M Snell>
or should it always be resolved in the context the stream was created... or should it always be resolved in the context that happens to be current whenever start/pull or whatever mechanism internal to the stream causes the promise to be rejected/resolved
18:48
<Mathieu Hofman>
Here is a question: if the user aborts, which causes the read to be rejected, what context matters? the context in which read was started, or the one in which the operation was aborted. I believe the latter is the meaningful one
18:49
<James M Snell>
I agree. but in the unhandledrejection handler for an individual read, does the same answer apply?
18:51
<James M Snell>
I can see arguments for both. For instance, if it's the runtime that ends up aborting the stream, my unhandledrejection handler might want to log request specific details of the read that was canceled, but I've lost that context because the abort came from the runtime in the root async context where no data was captured
18:52
<Mathieu Hofman>
I can see how losing track of the call site could be problematic in some use cases, but that's basically amounting to say that we have 3 contexts that can be meaningful: the operation start, the operation resolution, the registration for the result of the operation.
18:52
<James M Snell>
whereas within the cancel algorithm inside the stream itself, I may not care so much about which context was actually propagated
18:53
<James M Snell>
fwiw, I'm perfectly fine with us maintaining that it's the context that called the abort which is important
18:54
<James M Snell>
if someone really wants to preserve the context for the rejection then they need to explicitly handle the rejection
18:54
<Mathieu Hofman>
I have use cases where I'd like to be able to recover the context of the promise resolution
18:55
<Mathieu Hofman>
I can very well imagine some other people may have use cases where they need to recover the context of the promise creation
18:55
<James M Snell>
for my original case, however, the Response object is created in association with a context but used outside of that context and the user has no means of attaching rejection handlers to the internal promises so I think having the Response capture the context and propagate it makes the most sense
18:55
<Mathieu Hofman>
(to be clear, that on top of having the context of the promise handler registration)
18:57
<Mathieu Hofman>
I can see this being expensive to implement however