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
The actual pipe from the readable happens by the code that calls the What value would you expect |
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)> |
|
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 |
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:
|
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:
|
18:29 | <James M Snell> |
|
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..
|
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 |