Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add web integration document #100

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

andreubotella
Copy link
Member

No description provided.

[`requestVideoFrameCallback()`](https://wicg.github.io/video-rvfc/#dom-htmlvideoelement-requestvideoframecallback)
method [\[VIDEO-RVFC\]](https://wicg.github.io/video-rvfc/)

### Async completion callbacks
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Node's legacy APIs also fit into this category - is it worth mentioning them here, despite them not actually being web APIs?

What about a discussion of the transformation from callback to promise to async-await, and the effect that has on the continuation context as a function of the choice of callback snapshot or await restoration? TL;DR as I see it is that our choice here of always restoring callbacks to the registration-time snapshot makes these transformations consistent, but IIUC this is a departure from ALS's behavior, which led to some expectation of flow-through in order to make the transformation behave the same when the callback isn't restored. May or may not make sense to include here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node.js may choose either way--it can decide for itself whether it feels that the alignment with web or ALS is more important for them. I think it'd be good for someone to work on a separate design document for this, but I don't think it's a Stage 2.7-blocker the same way as web integration is.

Copy link
Member Author

@andreubotella andreubotella Jul 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this document is specifically about the web integration, because that is a requirement for stage 2.7, I think it should also apply to runtime-specific APIs. Otherwise, that would create an inconsistency in Node.js, since it will have to follow the web API behavior for the web APIs it implements.

I haven't looked at Node.js's APIs in any detail, but if you're talking specifically about async completion callbacks, I don't see how there would be any other possible context for the callbacks other than the time they're passed to the API. After all, the API (let's say fs.readFile(), for a concrete example) would start an async operation in the background, and the data flow of that operation would go back to the call to fs.readFile(). I don't think there is any other relevant context here, but @Qard can correct me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think Node.js can make this tradeoff for themselves, even as I acknowledge the inconsistency risk.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea was that fs.readFile might set some variables internally, and so there's a question of whether those changes would be reflected in the callback. @Qard seemed to suggest that they should, but this is precisely what leads to the inconsistency between callbacks and promises.

Copy link
Member Author

@andreubotella andreubotella Aug 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only observable if fs.readFile does in fact set variables internally. As far as I'm aware, this is not the case for any such APIs in Node.js.

But you're right that a lot of this text doesn't take runtime- (or web platform-) -internal variables into consideration. And things like the WebIDL handling of callbacks is described as simply storing the snapshot and restoring it later, when the needs of scheduler.yield would mean that very often we'd have to store a different snapshot where the scheduler.yield variable would change.

WEB-INTEGRATION.md Outdated Show resolved Hide resolved
WEB-INTEGRATION.md Outdated Show resolved Hide resolved
WEB-INTEGRATION.md Outdated Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this document. I overall am happy with the direction of choosing simple registration-time semantics for everything.

My concerns, in descending order of importance:

  • I am worried about the burden this will place on web spec authors.

    At first I thought this burden would be minimal, and web spec authors would need to drop down to async snapshot manipulation about as often as they have to drop down to other raw ECMAScript operations: i.e., extremely rarely, only for very special platform features or for patterns that aren't yet codified well into Web IDL. It seemed like auto-capturing for promises and Web IDL callbacks would generally be enough.

    But, the section on "Editorial aspects of AsyncContext integration in web specifications" seems to instead say that web spec authors will often have to do these manipulations. Or at least think about whether or not to do these manipulations. Maybe even as often as: every time they queue a task, every time they call a callback that might error, and every time they fire an event.

    That seems really bad. I am hopeful I am misunderstanding or there are some confusions here. I think clarifying which of the APIs under "Individual analysis of web APIs and AsyncContext" would need spec patches to use these operations, and what sort of patches they need, would be a good step toward clearing up this confusion.

  • There is a lot of discussion about future extensions to add more context tracking. This kind of unknown future complexity is a bit worrying. How seriously should we be preparing for this future complexity, on the web side? Is there more up-front work to do here, so we can be sure it'll work out? Or would it be OK if we found out later this was unworkable, and we never got anything besides registration-time contexts? (Plus, I guess, a couple that the proposal seems to want to bake in from the start, on error and unhandledrejection events.)

  • The document seems to have some editing mistakes per https://github.com/tc39/proposal-async-context/pull/100/files#r1688797847 regarding some dispatchSnapshot concept. That is pretty confusing and it'd be good to clear that up. IIUC that concept is gone, but maybe some sections are still discussing it? Similarly there's talk about the rejection context, but the proposal is to only focus on registration time, so why does rejection context matter?

WEB-INTEGRATION.md Outdated Show resolved Hide resolved
`AsyncContext.Snapshot` argument to the methods. This might be left for later,
rather than being part of the initial rollout.

- [`NavigateEvent`](https://html.spec.whatwg.org/multipage/nav-history-apis.html#navigateevent):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not clear what the takeaway is for this item.

getters of the returned object (e.g. the `processor` method in web audio
worklets, or the `attributeChangedCallback` method of custom elements) also
count as callbacks that these APIs invoke, and which should preserve the
relevant context.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is somewhat related to the old issue whatwg/webidl#701: the way in which these are specified today, by using raw JS machinery instead of a standardized Web IDL pattern, is annoying to get right and error-prone.

Certainly not a blocker, but if someone wanted to shave that yak while they were doing the web integration here, there would be much rejoicing.

WEB-INTEGRATION.md Outdated Show resolved Hide resolved
WEB-INTEGRATION.md Outdated Show resolved Hide resolved
WEB-INTEGRATION.md Outdated Show resolved Hide resolved
WEB-INTEGRATION.md Outdated Show resolved Hide resolved
WEB-INTEGRATION.md Outdated Show resolved Hide resolved
WEB-INTEGRATION.md Outdated Show resolved Hide resolved
WEB-INTEGRATION.md Outdated Show resolved Hide resolved
it. Therefore,
[`ErrorEvent`](https://html.spec.whatwg.org/multipage/webappapis.html#errorevent)
will have a `throwSnapshot` property, reflecting the context in which the
exception was thrown.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd appreciate mentioning the alternative to capture the context where the error constructor was called, and allowing developers set any context snapshot appropriate for their cases.

@shicks
Copy link
Contributor

shicks commented Aug 6, 2024

@domenic

  • I am worried about the burden this will place on web spec authors.
    At first I thought this burden would be minimal, and web spec authors would need to drop down to async snapshot manipulation about as often as they have to drop down to other raw ECMAScript operations: i.e., extremely rarely, only for very special platform features or for patterns that aren't yet codified well into Web IDL. It seemed like auto-capturing for promises and Web IDL callbacks would generally be enough.
    But, the section on "Editorial aspects of AsyncContext integration in web specifications" seems to instead say that web spec authors will often have to do these manipulations. Or at least think about whether or not to do these manipulations. Maybe even as often as: every time they queue a task, every time they call a callback that might error, and every time they fire an event.
    That seems really bad. I am hopeful I am misunderstanding or there are some confusions here. I think clarifying which of the APIs under "Individual analysis of web APIs and AsyncContext" would need spec patches to use these operations, and what sort of patches they need, would be a good step toward clearing up this confusion.

Between this and discussions w/ various frameworks (@rictic on Web Component integration, as well as some colleagues trying to use this both in our internal framework and in Angular), I think the thing to do may be to back off from the simplicity/consistency of "all builtin APIs always snapshot at registration time" to instead have two possible behaviors: (1) registration-time snapshotting, and (2) no context swapping at all, with the latter being the default unless it's more likely to be wrong in most cases (e.g. setTimeout). This is both more relevant for most APIs, and is less burdensome for web spec authors, since it's what will already happen in most cases, and/or what would fall out from just specifying how the swapping behaves outside of code execution contexts (i.e. in the event loop).

@domenic
Copy link
Member

domenic commented Aug 6, 2024

I don't understand how adding an extra choice (registration time vs. not) would make it less burdensome for web spec authors...

@shicks
Copy link
Contributor

shicks commented Aug 6, 2024

I think in general the "do nothing" default would be correct.

@andreubotella
Copy link
Member Author

andreubotella commented Aug 6, 2024

Between this and discussions w/ various frameworks (@rictic on Web Component integration, as well as some colleagues trying to use this both in our internal framework and in Angular), I think the thing to do may be to back off from the simplicity/consistency of "all builtin APIs always snapshot at registration time" to instead have two possible behaviors: (1) registration-time snapshotting, and (2) no context swapping at all, with the latter being the default unless it's more likely to be wrong in most cases (e.g. setTimeout). This is both more relevant for most APIs, and is less burdensome for web spec authors, since it's what will already happen in most cases, and/or what would fall out from just specifying how the swapping behaves outside of code execution contexts (i.e. in the event loop).

Are you saying that things like the XHR load event should not have any way to get back to the xhr.send() context? Because that is the kind of thing that would not be handled automatically by WebIDL in the current proposal, and that would need to be handled by spec authors. Unless we make it so queueing a new task in the event loop always propagates the snapshot, even when going "in parallel" (i.e. spawning a thread to avoid blocking the event loop), which I suspect is a whole 'nother can of worms.

@yoavweiss
Copy link

I haven't dug deep into the discussion here, but in case it's useful for y'all, I sketched out the web integration of Task Attribution at https://wicg.github.io/soft-navigations/#sec-task-attribution-algorithms last year.

@shicks
Copy link
Contributor

shicks commented Aug 6, 2024

@andreubotella

Are you saying that things like the XHR load event should not have any way to get back to the xhr.send() context? Because that is the kind of thing that would not be handled automatically by WebIDL in the current proposal, and that would need to be handled by spec authors. Unless we make it so queueing a new task in the event loop always propagates the snapshot, even when going "in parallel" (i.e. spawning a thread to avoid blocking the event loop), which I suspect is a whole 'nother can of worms.

I think XHR is one of those cases that will need to be handled specially no matter what we go with as a default. Ultimately, getting the right default matters a lot, because it's important that user code shouldn't need to be aware of AsyncContext at all. It's okay if framework code needs to read snapshots off of event objects (assuming it's got access to them in the right place), but if user code needs to do it, then we've got a problem.

I don't know how bad it is to say that XHR events have a different default context than other events. I guess what it would look like is that events would need to either (1) run their handlers in the current context - i.e. no swapping; or (2) run their handlers in a given context that's maybe initialized with the event, and XHR events would set this to the load or send snapshot, error and unhandledrejection events would likely set this to the error/rejection snapshot... unfortunately this does mean we need to figure out all the web events upfront, which is something I was really hoping we could avoid.

@andreubotella
Copy link
Member Author

andreubotella commented Aug 20, 2024

Hey, sorry everyone for the radio silence over here. We've been discussing some of the details of the web integration in the AsyncContext Matrix channel and in our biweekly meetings, and here's a summary of the current state of things:

For events, the web integration proposed in this document would call event listeners with the registration context, which would be propagated automatically by WebIDL (through browser- and spec-internal machinery). The dispatch context would be exposed as a property on a limited set of event subclasses, where that set would start intentionally small and could grow over time as more use cases are identified without breaking backwards compatibility. It is in these dispatch context cases where web spec authors would need to manually track the data flow.

However, @shicks, @rictic and @jatraman pointed out that using the registration context doesn't work for their use cases, where the AsyncContext user would be a third-party library that needs to track the context through event registrations in first-party code. When the first-party code eventually calls into the third-party library, that library would need to access the event's dispatch context to associate the event with its source, which in the current proposal would need the first-party code to explicitly pass the dispatch context, potentially across multiple function calls – which is the very pain point that AsyncContext aims to prevent in the first place!

The alternative option for events would be to always call the listeners with the event's dispatch context if there is one, and to use the initial AsyncContext snapshot otherwise1. For sync dispatch contexts this would not require any extra work in specs or browsers, since the value of the agent's [[AsyncContextMapping]] field when the event is fired would be correct. Similarly, for event dispatches without a dispatch context (i.e. events not caused by JS, such as a user click), [[AsyncContextMapping]] would be set to its default value, which is the initial snapshot. The problem is with async dispatch contexts.

When a web API called from JS eventually causes a task to be queued on the event loop which fires an event, the context that called the web API should be the (async) dispatch context of that event. A common example of this is XHR, where the synchronous call to xhr.send() starts an asynchronous fetch which eventually fires the e.g. load event. And currently there is no mechanism in web specs or browsers that could automatically propagate the [[AsyncContextMapping]] across async tasks, so the event listener would have the initial AsyncContext snapshot by default, which would not be the right context.

It's not just XHR, there seem to be many cases where events are caused by a web API but not fired until later. We don't have a full list of such events, because in non-trivial cases, knowing whether an event has an async source requires tracing through the data flow in the spec text or in browser code. So properly tracking the async sources for every event is unfeasible if we want to get AsyncContext shipped anytime soon.

@shicks's proposal, as I understand it, is to have a small set of events with async sources which would be handled in the initial rollout, and for any event that isn't handled, the initial AsyncContext snapshot would be used. One big issue with this compared to the current document is that this set would be very hard to extend in the future, since developers might depend on the context for a specific event being the initial one, so changing that context might mean breaking the web. It would also be quite hard for browsers to get telemetry about the usage of such contexts, which makes it more risky to changing the context in the future.

It seems like there are no great solutions, and the fact that the users of these events would typically be first-party developers, whereas AsyncContext would be used in third-party libraries, makes it even harder to have a clear idea of the use cases we need to support.


TL;DR: We're considering changing the context in which event listeners are called to the dispatch context, not the registration context. Handling this properly for all events would require such amount of work in the specs and browsers that it would probably be unfeasible. But if we only handle a subset of events in the initial rollout, the rest of events might not be able to be fixed in the future without breaking the web.

Footnotes

  1. Or alternatively, the registration context could also be stored and used when there is no other context.

@andreubotella
Copy link
Member Author

andreubotella commented Sep 9, 2024

As an update: we're currently looking into whether it's possible to hook into the HTML spec's event loop mechanisms, in particular "queue a task" and "in parallel" to automatically propagate the context across any JS code execution in the same agent that is caused, directly or indirectly, by a previous execution. Doing this would automatically make most events that are dispatched asynchronously have the right async dispatch context, so most spec authors would not have to worry about it.

However, it is not fully clear to us how hard it would be to implement this in browsers, and whether it can be implemented in a way such that most browser contributors also don't have to worry about it looking forwards. This gets even more complicated when you take into account the difficulty of somehow propagating contexts behind the scenes across cross-thread and cross-process data flows, while making sure the snapshot and the data contained get properly GC'd.

@andreubotella
Copy link
Member Author

I updated this document to prefer using the dispatch context, and only use the registration context if the API in question can never have a dispatch context. I replaced most of the event sections with a TODO that @nicolo-ribaudo will fill with their fallback context proposal. I also added the automatic context propagation to the editorial aspects section, which will be added to more in the future as we figure out the details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants