Repository navigation
Reviving "Zones" like proposals to TC39 #375
Description
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.
Activity
mmarchini commentedon Apr 8, 2020
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 commentedon Apr 15, 2020
@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 commentedon Apr 15, 2020
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 commentedon Apr 15, 2020
There's some discussion about maybe having this handler as part of the agents proposal.
mmarchini commentedon May 1, 2020
@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.
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 withinfn. IMO it should steer away from always reporting toHostReportErrors, and instead it should retain the current behavior as iffnwas not wrapped (meaning iffnwouldn't report toHostReportErrorswhile not wrapped, it wouldn't while wrapped, same goes to if it would report toHostReportErrors). Since Promise rejections always report toHostPromiseRejectionTrackerI 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 commentedon May 2, 2020
@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.
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 commentedon May 6, 2020
@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 commentedon May 6, 2020
Thanks for setting this up. I suggest sharing the result here once we have a date so folks don't miss it :)
legendecas commentedon May 9, 2020
So after a few of us have filled the doodle, we have only one timeslot that fit all of us:
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 commentedon May 12, 2020
After we discussed in the meeting, we have the following todos:
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 commentedon May 20, 2020
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 commentedon May 20, 2020
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 commentedon May 20, 2020
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 commentedon May 20, 2020
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.
5 remaining items
Qard commentedon May 22, 2020
The
throwdoes 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 commentedon May 22, 2020
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. 😅
benjamingr commentedon May 22, 2020
Flarna commentedon May 22, 2020
There is no difference in
err.stackbut if you run above script you can see a difference in the first line of the uncaught exception message on console which points to thethrow eline:Also debugger points to the re-throw location.
benjamingr commentedon May 22, 2020
@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 commentedon May 23, 2020
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. SoAsyncResourcecan be essential to link logical async contexts correctly, and propagate async locals through AsyncLocalStorage correctly. Basically, wrappings can be as simple asin Node.js, so I believe it will not be an issue if
runInAsyncScopeis properly defined.legendecas commentedon Jul 8, 2020
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.rununnecessary to have the value in the store propagates along with the async flow, like what we can have withThreadLocal.I'd like to request for more reviews on the current proposal https://github.com/legendecas/proposal-async-context.
Flarna commentedon Jul 9, 2020
@legendecas Please note that an API including an
valueChangedListenerwas 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
AsyncTasksample thethis.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
setIntervalwill 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 commentedon Jul 9, 2020
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 commentedon Jul 9, 2020
What was the motivator for
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 commentedon Jul 12, 2020
Thank you all for the reviews!! @Flarna @Qard @mhdawson
This is a very helpful history. However, there are differences between the proposed
valueChangedListenerand 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 thevalueChangedListener, rather by the time the user explicitly set the value byasyncLocal.setValue(value). So it is rather simple a setter function of the AsyncLocal.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
asyncIdandtriggerAsyncIdstuff.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 currentAsyncLocalvalue propagation can be fulfilled by leveraging the performance hit to the time when we explicitly callAsyncLocal.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 toundefinedis sufficient in ways to stop the value propagation.Also, it is notable that
AsyncLocalStorage.exitin Node.js can be treated asAsyncLocal.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.Yes, thanks for the reminder. Appended https://github.com/legendecas/proposal-async-context/blob/master/SOLUTION.md#how-the-value-got-propagated .
github-actions commentedon Nov 4, 2020
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.