Reviving "Zones" like proposals to TC39

[legendecas]

Since original zones proposal had have been withdrawn and its repo has been archived, I've forked another repo on the zones proposal at https://github.com/legendecas/zones and reset to the proper commit. Discuss on this thread is also fine.

The motivation of the proposal can still focus on async contexts in JavaScript, i.e. (1) tracking host tasks scheduled during a period of execution, (2) provided an easy way to inspecting current executing host tasks and its callbacks/promise resolutions, (3) enable toolings that work around the tasks like profiling on the async task run time. Also, common pitfall issues raised in the Node.js domain module, like globally shared instance without isolation between modules, semantically misleading error handling, have to be prevented/addressed in the new proposal too.

@mmarchini mentioned in the last diag meeting that there are a few outstanding proposals that may be overlapping the motivation of the "zones" like proposals. Options can be seeking to promote and work on these proposals too.

[mmarchini]

Reading the proposal again I don't think there's overlap, but we'll need to think about interoperability with other proposals. It seems like the proposal is blocked due to an objection on domenic/zones#9. The thread is huge and I haven't read it yet (will do it in the following days), but without addressing the objections there we can't move forward.

[legendecas]

@mmarchini After reading many threads on the zones proposal, IMO I'd like to keeping the proposal minimal and staying away from error handling at the moment. JavaScript has its own error handling semantics on try...catch, introducing another one the way that Zones do doesn't seem to make the error handling easy to understand.

What's your opinion on what the Zones proposal shall do?

[Qard]

I feel like async/await with try/catch already adequately solves most error handling concerns. The main exception being forgetting to await a promise, but the uncaught exception handler can already deal with that. Might be worth just making a spec to expose the uncaught exception handler somehow. 🤔

[mmarchini]

Might be worth just making a spec to expose the uncaught exception handler somehow

There's some discussion about maybe having this handler as part of the agents proposal.

[mmarchini]

@legendecas are you actively working on the zones proposal (or do you plan to work/have cycles to work in the near future)? I've read the proposal as well as domenic/zones#1 and domenic/zones#9 (the biggest blockers), and I have ideas on how to advance it. If you're too busy right now I can champion it.

After reading many threads on the zones proposal, IMO I'd like to keeping the proposal minimal and staying away from error handling at the moment.

I agree with that, after reading the issues and the domains postmortem, IMO introducing error capturing as part of zones will open the door for misuse of zones as a "catch-all" mechanism, which is far from the goal of the proposal. But note that .wrap(fn) requires a decision on how it will handle errors that happen within fn. IMO it should steer away from always reporting to HostReportErrors, and instead it should retain the current behavior as if fn was not wrapped (meaning if fn wouldn't report to HostReportErrors while not wrapped, it wouldn't while wrapped, same goes to if it would report to HostReportErrors). Since Promise rejections always report to HostPromiseRejectionTracker I think it's less on an issue on the Promise and async/await side.

While I believe error handling and error capturing should not be part of the proposal, I think zones brings an opportunity to improve the concept of async stack traces introduced by V8: instead of limiting async stack traces to async/await, we might be able to expand it across zones. This would be a third proposal though, derived from zones and Error.stack.

[legendecas]

@mmarchini yes, I'm on the process on TC39 delegation as part of TC39 proposal working procedure. Besides that, I've proposed a draft on https://github.com/legendecas/proposal-async-context (this is an early stage draft, ideas are welcome), in which I've listed the error handling as non-goal of the proposal.

While I believe error handling and error capturing should not be part of the proposal, I think zones brings an opportunity to improve the concept of async stack traces introduced by V8: instead of limiting async stack traces to async/await, we might be able to expand it across zones. This would be a third proposal though, derived from zones and Error.stack.

This is cool and may be one of the main motivations for the proposal. I'll update the draft to address this.

Championships are very welcome if you and any person are interested in working on this together.

[legendecas]

@mmarchini and other people may be interested since this topic is quite a large one, I'm proposing a separate online call session to discuss priorities and shape of this early-stage proposal. Anyone interested can fill out this doodle https://doodle.com/poll/r3sxeaevzcm8gtdx, so that we can pick a perfect time for all of us.

[mmarchini]

Thanks for setting this up. I suggest sharing the result here once we have a date so folks don't miss it :)

[legendecas]

So after a few of us have filled the doodle, we have only one timeslot that fit all of us:

Location	Date
Los Angeles	May 12 TUE 9:00 AM - 10:00 AM
New York	May 12 TUE 12:00 PM - 1:00 PM
UTC	May 12 TUE 16:00 PM - 17:00 PM
Hangzhou	May 13 WED 12:00 AM - 1:00 AM

Meeting agenda and notes: https://docs.google.com/document/d/1459W_1uCnrrprTcqpxzw315e4ceVrYJt4xVdJRbxGYk/edit?usp=sharing
Meeting link: https://alimeeting.alibaba-inc.com/alimeeting/web/webvc/alilangpc?uuid=9f3db55a-9061-49c8-877f-e2ae01a13719 (you can join with browsers)

[legendecas]

After we discussed in the meeting, we have the following todos:

• The Cancellation proposal is on stage 1, should be taken in to account.
• Adopt AsyncLocalStorage into the proposal or change the implementation.

The original zones proposal was on stage 0 (tc39/proposals@2764565), so we can rename the new proposal to fit our motivations.

@mhdawson @mmarchini @Qard thank you for your suggestions!

[legendecas]

hey, @benjamingr @bmeck @domenic @trevnorris :D, you have lots of comments and thoughts on original zones proposal repository. So before we are going to present the new async context proposal to TC39 in the very near future, may I have your attention and ideas on this new proposal?

Compared to the Zones proposal, the new proposal is kept minimal and providing a way to track async contexts and async locals. While error handling and task interception are not the priorities and requirements on the proposal after all the features like async/await in JavaScript, I'm wondering if the blocking issue of such an area of proposal is cleared from your point of view.

[domenic]

I am no longer involved in TC39. But from my perspective any proposal which doesn't have error handling affordances (notably the two ways to wrap/propagate contexts) is not likely to succeed, and would be bad for the ecosystem if it did.

[mmarchini]

After reading all the discussions in the original zones repository, I have an opposing opinion: a proposal with error handling is unlikely to succeed, because it mixes error handling and mapping asynchronous continuity (two different concepts). A separate proposal for "error capturing context", which is not limited to async contexts (but take those into account) might be more appropriate.

[benjamingr]

I am and have always been in favor of zones without error handling.

I find the concept genuinely useful and I find myself many times looking for something like zones (which I use async_hooks APIs for).

I am also +1 on zones with error handling if we address the issues that made domains fail - for example by the zone being able to get notified of an error but not "handle" it.

[benjamingr]

Would your proposal be a little like pulling Node's recent AsyncLocalStorage into JavaScript?

If so, I am in favor and think it could solve a need developers have had for a while and worked around with a fragmented ecosystem.

[legendecas]

Would your proposal be a little like pulling Node's recent AsyncLocalStorage into JavaScript?

Yes, I believe this is a critical feature to introduce "logical async context" to JavaScript.

[benjamingr]

How would wrapping work? Is the idea to bring AsyncLocalStorage in but keep AsyncResource and ways to set up content (Zone.run) in the platforms? Is the idea to have a Zone.run/wrap analog but not have wrapGuarded/runGuarded ?

[Qard]

👍 for AsyncLocalStorage copied mostly as-is. It wouldn't be too hard to layer error-handling concepts on later, but I do think they are separate enough that they should not block the context propagation capability.

My main concern with attaching error-handling to it is that multi-tenancy gets very complicated. It'd need the ability to rethrow an error, or let it flow through somehow, without tainting the original information as an outer container might be expecting it to look a certain way.

On a related note: I have been thinking it'd be nice to have a "clean" rethrow feature of sorts like try { ... } catch (e) { rethrow } that'd return to unwinding the stack for the currently intercepted error without modifying the trace information on it. Could actually help a bunch with making the need for conditional error handling here more reasonable. 🤔

[benjamingr]

To be honest - I'm not nearly as concerned with error handling as I was 4-5 years ago. The biggest issue domains had was IMO:

foo((err, data) => {
  someLibrary((err, data) => {
    bar((err, data) => {
        doSomethingThatThrows();
    });
  });
});

Here if bar throws it's possible someLibrary intercepts the error and foo can't necessarily be notified or run cleanup code (if it assumes it does because of domains). That's a very real issue IMO and part of why Node was blocking zones. On the other hand - Angular's main reason for not wanting zones without error handling was that without error handling in other cases errors didn't propagate correctly. Both parties make a good point.

In retrospect I'm not sure what was wrong with letting zones notice errors (so Angular's use case is handled) but not intercept them (so Node's is)?

That said - AsyncLocalStorage addresses all of the things I actually want from zones without any of the downsides.

@Qard

that'd return to unwinding the stack for the currently intercepted error without modifying the trace information on it.

Can you explain what that means and how that would be different from } catch (e) { throw e; }? In C# there's a special syntax for this - but only because in C# the stack trace of an error is determined when it is thrown - in JS that's not the case as far as I know.

[Qard]

The throw does some extra stuff to imprint the given error with the location that throw occurred. That means throwing something that has already been thrown will mess with the stack information such that the error then points to the new throw point not the original. This is an observable difference so it would be nice to be able to bail out after some quick checks to see the error can not be handled at that level after all. 🤔

[Qard]

Oh, actually, I think you're right about that not mattering in JS. The stack trace output remains the same. I must've been mis-remembering. 😅

[Flarna]

There is no difference in err.stack but if you run above script you can see a difference in the first line of the uncaught exception message on console which points to the throw e line:

C:\work\xxx\index.js:11
    throw e;

Also debugger points to the re-throw location.

[benjamingr]

@Flarna should we potentially fix that or is that behavior desirable? AFAIK that's something Node does and that information (where the last throw was) isn't exposed by the JS language itself (and IIRC stacks aren't actually specified and where they get populated isn't either there was a proposal at one point).

[legendecas]

How would wrapping work? Is the idea to bring AsyncLocalStorage in but keep AsyncResource and ways to set up content (Zone.run) in the platforms? Is the idea to have a Zone.run/wrap analog but not have wrapGuarded/runGuarded ?

Since we are not going to include error handling in this minimal proposal (may be in a follow up proposal), there will be no wrapGuarded/runGuarded. AsyncResources are useful when there is userland task scheduling on orchestrating multiple hosts provided async resources underlying. So AsyncResource can be essential to link logical async contexts correctly, and propagate async locals through AsyncLocalStorage correctly. Basically, wrappings can be as simple as

function wrap(asyncResource, callback) {
  return (...args) => asyncResource.runInAsyncScope(callback, undefined, ...args)
}

in Node.js, so I believe it will not be an issue if runInAsyncScope is properly defined.

[legendecas]

Since the last TC39 meeting on June, there are a bunch of updates on the async context proposal.

The major changes are removing the hooks mechanism from the proposal and leave it to the host platforms like Node.js, and added a strictly restricted value changed listener on each AsyncLocal instance to address the security concerns. And renamed the AsyncLocalStorage to AsyncLocal to prevent naming confusions with LocalStorage on web platform. Also make the manual  AsyncLocalStorage.run unnecessary to have the value in the store propagates along with the async flow, like what we can have with ThreadLocal.

I'd like to request for more reviews on the current proposal https://github.com/legendecas/proposal-async-context.

[Flarna]

@legendecas Please note that an API including an valueChangedListener was already proposed a while ago for Node and rejected (see nodejs/node#27172) as "to dangerous".
The listener should be at least optional and it should be clearly state that it is not allowed to set a new value within the listener or throw.

Why is in the AsyncTask sample the this.socket.send() part not associated to this new Task?

What I miss is some sort of breaking/stopping propagation based on a condition. For example setInterval will bind a context forever.

Please add some more details how the value is propagated by AsyncLocal. I assume it's just assignment, no deep copy. This enables users to put a value into an object property which would be visible to parents.

[Qard]

It's worth pointing out that while an engine might naively just apply context linkage based purely on promises and the microtask queue, if they expose internals as a library in a way which something like Node.js can provide it's own asyncrony, it will likely need native API changes to handle binding a callback to a context. This has public API surface implications which most JS language specs don't have, so that should be clearly communicated.

[mhdawson]

What was the motivator for

Also make the manual AsyncLocalStorage.run unnecessary to have the value in the store propagates along with the async flow, like what we can have with ThreadLocal.

Whether to require a run or not was discussed a fair amount as part of the Node.js ALS so it would be good to understand why it would not be part of the TC39 proposal. As mentioned by @Flarna its probably needed (in addition to other methods) to control breaking/stopping propagation.

[legendecas]

Thank you all for the reviews!! @Flarna @Qard @mhdawson

@Flarna: Please note that an API including an valueChangedListener was already proposed a while ago for Node and rejected as "to dangerous".

This is a very helpful history. However, there are differences between the proposed valueChangedListener and the solution in nodejs/node#27172. The key difference between them is the time when they will be called. As the one in the proposal, the listener will not be called on any phrase of the async resource lifetime, it is safe to use any host provided asynchronous API in the valueChangedListener, rather by the time the user explicitly set the value by asyncLocal.setValue(value). So it is rather simple a setter function of the AsyncLocal.

@Flarna: Why is in the AsyncTask sample the this.socket.send() part not associated to this new Task?

I've updated the AsyncTask example to better illustrate the problem that AsyncTask is going to address. I'd believe AsyncTask is pretty much similar to AsyncResource in Node.js without asyncId and triggerAsyncId stuff.

@Flarna: What I miss is some sort of breaking/stopping propagation based on a condition
@mhdawson: Whether to require a run or not was discussed a fair amount as part of the Node.js ALS so it would be good to understand why it would not be part of the TC39 proposal.

The propagation operation in the section doesn't mean an actual propagation operation will be effective immediately. It is possible to link those async operations by the time of invocation like Promise.resolve(value). As such, one of the major design goal of current AsyncLocal value propagation can be fulfilled by leveraging the performance hit to the time when we explicitly call AsyncLocal.getValue(). The applications that didn't use any AsyncLocal features will not be punished for the implementation of AsyncLocal.

As the propagation is designed to be lightweight enough as long as there is no actual AsyncLocal.getValue() been called, setting the value of AsyncLocal to undefined is sufficient in ways to stop the value propagation.

Also, it is notable that AsyncLocalStorage.exit in Node.js can be treated as AsyncLocal.setValue(undefined) + AsyncTask.runInAyncScope() + AsyncLocal.setValue(originalValue). So I'd believe there is no significant requirement to add an explicit stop propagation method to AsyncLocal.

@Flarna: Please add some more details how the value is propagated by AsyncLocal. I assume it's just assignment, no deep copy.

Yes, thanks for the reminder. Appended https://github.com/legendecas/proposal-async-context/blob/master/SOLUTION.md#how-the-value-got-propagated .

[github-actions]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.