-
Notifications
You must be signed in to change notification settings - Fork 322
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
Initial implementation of async context tracking #208
Conversation
839e276
to
1ab5c3f
Compare
Is there risk in shipping non-standardised APIs from a different JS environment? This feels unprecedented in Workers. |
Yes, definitely. We plan to ship implementations of a subset of Node.js APIs gated by a compatibility flag to explicitly opt-in to their use and requiring import/require to use them. There is certainly an amount of risk involved. |
a74f816
to
6df0c43
Compare
2103091
to
fc18885
Compare
81ea14c
to
f1c48ed
Compare
87a5e19
to
232b511
Compare
Fixups were squashed, commits rebased to resolve conflicts, new commit added to propagate context to unhandledrejection error. |
f2cdbb0
to
e3fe065
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm about halfway done reviewing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished first review pass.
src/workerd/api/node/async-hooks.c++
Outdated
// Serves the same purpose as attach() in KJ things. Ensures that we hold a reference | ||
// to the AsyncResource object wrapper for as long as the function is held. | ||
jsg::check(bound->SetPrivate(js.v8Isolate->GetCurrentContext(), | ||
v8::Private::ForApi(js.v8Isolate, jsg::v8StrIntern(js.v8Isolate, "ref")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry that "ref" is an awfully generic name for this. I suppose the likelihood of collisions in our usage of private symbols is low since we don't really use them anywhere yet (except here), but I wonder if we should be namespacing them better in some way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, if we can come up with a reasonable convention now I'm happy to use it, otherwise it is a potential problem that we can solve later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, the overall design here is something like this:
- An
AsyncResource
stores the current mapping of ASL IDs to values. - At any given time there is a "current"
AsyncResource
, which is used to implementals.getStore()
. als.run()
doesn't create a new map. It actually modifies the map in the current resource's map, then runs the callback, then reverts the change to the resource.- When the context needs to be inherited into an async callback (e.g. a promise continuation), this is always accomplished by making a whole new
AsyncResource
, which makes a full copy of the whole current storage map. The continuation can't just hold on to the current resource, since the value set byals.run()
would be reverted before the continuation actually runs.
I would have expected a design more like this:
- The mapping of ALS keys to values is held by an "async context frame".
als.run()
creates a new frame, inheriting the contents of the current frame, but changing the one value. The callback runs with the new frame set "current". Upon return, the thread reverts to the previous frame being current.- Promise continuations inherit the current frame from the point where
.then()
is called. This simply references the existing frame, there is no need to allocate a new object or copy anything, since each frame is immutable once it is constructed.
This approach would be much more efficient, as it doesn't require any allocations except when the ALS API is actually used.
(Note that my comments below are more tactical and mostly written before I really understood the overall design as discussed above.)
I would recommend not getting too hung up on the naming of this construct. If it makes more sense to you calling it an "async context frame" then ok. Given that this is part of the Node.js compatibility bit, I prefer terminology and a model that aligns with Node.js but the naming isn't something I consider to be important at all. That said, yes, this "async context frame" (or In the Node.js model, every
Put A single While a particular
Correct. This is exactly what we want.
Whenever a new So, for instance, an individual Let's assume that the current I create a new Promise using Then I create another new Promise using When we resolve the first promise by calling Now, whether or not the current implementation correctly captures this model, I'm not entirely sure because I haven't yet implemented all of the tests to verify. It's entirely possible that some of the details in the current implementation are wrong and will need to verified still. For example, given the following... let r;
const p = als.run(123, () => new Promise((res) => r = res));
p2 = p.then(() => { console.log(als.getStore()); });
r(); To match Node.js' model, the console.log should print
Again, const als = new AsyncLocalStorage();
let res;
const p0 = new Promise((r) => res = r);
const p1 = als.run(123, () => p0.then(() => { console.log(als.getStore()) }));
const p2 = als.run(321, () => p0.then(() => { console.log(als.getStore()) }));
res(); Here, in Node.js' model there are five distinct
The value associated with
Again, I wouldn't get too hung up on the naming. What you call
This is incorrect and is not what we want.
Again, a new promise is a new To summarize...
Item 5 is the key piece we need to verify here and optimize for. Verification will come as the set of tests are expanded (as I said, I'm not convinced this impl is quite right yet with regards to the promise hooks). Optimization will come after things are verified. To illustrate point 6, consider the example: als.run(123, () => {
setInterval(() => {
console.log(als.getStore())
}, 1000);
});
And this example: const als = new AsyncLocalStorage();
const fn = als.run(123, () => AsyncResource.bind(() => console.log(als.getStore())));
als.run(321, fn); // prints 123
als.run(321, fn); // prints 123
als.run(321, fn); // prints 123
als.run(321, fn); // prints 123 The function returned by |
James, I think you misunderstood what I was trying to say. I am suggesting an alternate implementation which achieves the same semantics but will be much more efficient. Your current implementation creates a new AsyncResource, and therefore creates a whole new copy of the entire ALS map, for every
No it isn't. I'm trying to describe a different implementation, which works differently. It's not a difference in naming, it's actually a different design.
Yes it does. This is a key difference in the implementation I'm proposing. The application-visible semantics are the same, but the approach avoids the need to do any additional allocation on |
170dbda
to
fa30035
Compare
d45c248
to
308e2fd
Compare
@harrishancock @kentonv ... updated to address review feedback. Please take a look. |
1be71fa
to
8b04de0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is looking a lot nicer now. Not done reviewing yet but here's a few comments.
JSG_NESTED_TYPE(AsyncResource); | ||
|
||
if (flags.getNodeJsCompat()) { | ||
JSG_TS_ROOT(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will put AsyncLocalStorage
and AsyncResource
in the global typing environment, whereas we want them in a module
like (assuming they're accessible via named exports):
declare module "node:async_hooks" {
export class AsyncLocalStorage { ... }
export class AsyncResource { ... }
}
This will need additional work in the type generation scripts.
I think we'll either want to add something like JSG_TS_MODULE("node:async_hooks")
which acts like JSG_TS_ROOT()
, but puts the visited types in a declare module
, or we could try call registerNodeJsCompatModules
in api-encoder
.
I'm happy to contribute this in a follow-up PR. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Built-ins using a JSG object do not yet supports named exports. Everything is imported via the default
currently... e.g.
import { default as async_hooks } from 'node:async_hooks';
I'll be hoping to get to named export support soon (hopefully next week).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I actually managed to read everything this time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a look at everything besides async-context and async-hooks. Will take a bit to understand those.
@@ -0,0 +1,23 @@ | |||
import { default as async_hooks } from 'node:async_hooks'; | |||
const { AsyncLocalStorage, AsyncResource } = async_hooks; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm. doesn't import AsyncLocalStorage, AsyncResource from ""
work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would if these were typescript modules. The built-in using a backing jsg::Object does not yet support named exports.
@kentonv ... I'd like to get this squashed down and rebased on current |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting closer.
f2c2598
to
2eee21f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would still be nice to add a test for the case of a wrapped function throwing an exception but the implementation looks good to me at this point.
That's added in the internal test. |
de260f3
to
a97d62f
Compare
f76e552
to
97d9501
Compare
Initial experimental async context tracking (internals only).
This is intended to set the stage for an eventual implementation ofAsyncLocalStorage
(and later the proposed TC-39AsyncContext
proposal that is based onAsyncLocalStorage
.)This initial PR seeks
onlyto add the internal book keeping for the async context, including implementing the basic v8 promise hooks, as well as attaching the async context tracking tosetTimeout
,setInterval
, andqueueMicrotask
.This PR
does notimplements theAsyncLocalStorage
API as well.Note that we eventually expect V8 to implement these core mechanisms as part of the
AsyncContext
proposal implementation as that moves forward in TC-39, but that will be some time off and there's no guarantee that proposal will advance through TC-39. The goal of this implementation here is to remain simple. It is modeled after Node.js' approach which is known to work very well.For the AsyncLocalStorage piece... the API itself is not yet exposed to JavaScript to use. That'll come later. When it does come, users will use an
import
orrequire
to utilize it:The ALS implementation will support the
als.run(...)
,als.exit(...)
, andals.getStore()
methods. Theals.enterWith()
andals.disable()
methods will not be implemented.TODOs
The implementation of AsyncLocalStorage here definitely is not yet correct in that each ALS instance does not yet maintain it's own storage context. Specifically, if we look at Node.js' implementation, multiple ALS instances can safely exist simultaneously without risk of interfering with each other's data. The implementation here currently does not have that protection. That needs to be addressed before is complete.Each ALS instance now maintains its own distinct storage cell.We should also implementAsyncResource
to allow for custom contextsCurrently we always enter the async context in run even if we are already in it. We can optimize things by checking to see if we're already within the context and if the given store is already the stored value.There's currently a challenge when a tracked promise is gc'd while it'sAsyncResource
is still on the stack causing a UAF.unhandledrejection
/rejectionhandled
events will pick up the async context of their associated promises.HtmlRewriter
--Looks like there is a non-trivial performance issue enabling this with HtmlRewriter as is. Will need to investigate how to make it not as costly. There's also a crash that I need to investigate when enabling this.Decide if theWill handle these separately in other PRS if we choose to do anythingIoContext
is itself an async resource so that we can differentiate between the top level async context (at global scope) and the context within a single request.EventTarget things (like WebSocket) could potentially also be async contexts. This would be inconsistent with Node.js, however, in that Node.js does not treat EventEmitter instances as async resources. Do we want to make things like WebSocket an async resource?Will handle these separately in other PRS if we choose to do anythingtest bin in workerdNot critical for landing, we have ew-tests internally. May add this with a separate PR