Repository navigation
Should EventEmitter extend AsyncResource? #34430
Description
Activity
I'm not an expert, but events are dispatched synchronously. So if you emit an event in some async code, the context should already be correct?
Not at all. Depends on what the root of the event is. e.g. most stream events would need a runinasynccontext when backed by e.g. a socket.
Do fs methods properly propagate async context?
I think this generally makes sense, for the same reason that the
domainmodule, in addition to tracking async context, also monkey-patched theEventEmitterclass to provide for this.Also, just for context, there is some previous discussion in #33723.
oh is this about propagating the context that was active when the emitter instance was created?
Reacted by Robert NagyThat’s a valid question. Either we propagate the context of when the emitter was created, or the context of when the listener was registered.
My understanding of how async_hooks works, and my understanding of what makes sense here for e.g. ALS, would be to use the async id of the listener registration, but the async trigger id of the emitter creation. In particular, for this:
http.createServer((req, res) => { asyncLocalStorage.run(store, () => { req.on('end', () => { const store = asyncLocalStorage.getStore(); }); }); }).listen(8080);
I think it would be a common expectation that the inner
storevariable matches the outer one.async id of the listener registration, but the async trigger id of the emitter creation
This has always confused me. What is the practical difference of these two? i.e. how would I consume them differently?
@addaleax Could
enterWith(store)be an answer here? Modifying your example:http.createServer((req, res) => { asyncLocalStorage.enterWith(store); req.on('end', () => { const store = asyncLocalStorage.getStore(); }); }).listen(8080);
On v12.18.2 the store is still available in the callback.
As far as I understandenterWith, the store should not leak to other requests, so would there be any reason not to use it like that?@kjarmicki That might work in your case, but it probably only does so by accident – in order to actually consume the content of
req, you’ll need to read fromreqor discard its contents usingreq.resume();, and at least as far as I can tell the deciding factor is whether the resume call happens inside the ALS scope or not. Event listeners don’t interact with ALS at all so far by themselves.@addaleax oh, ok 🙂
So the right way would be to wrap the
endevent with a Promise, correct?http.createServer((req, res) => { asyncLocalStorage.run(store, () => { new Promise(resolve => req.on('end', resolve)) .then(() => { const store = asyncLocalStorage.getStore(); }); }); }).listen(8080);
@kjarmicki I’m not sure what you mean by “the right way”, but that should work (and using
await events.once(req, 'end')would work as well). But this discussion here is specifically about whether EventEmitter should also propagate async context on its own.Reacted by Krystian Jarmicki@ronag My understanding is that the async id propagates the when of an async call, and the trigger async id propagates the why. I think we’re definitely lacking a clear definition that can be applied programatically here, though.
That answers part of my question. Though I'm still unsure when & how I would practically use e.g. trigger id. I think some motivating examples in the doc would be nice in this case.
Fwiw, I tried this once and performance of EventEmitter dropped tenfold. A better approach (albeit less ergonomic) would be to either emit from within the context you wish to propagate, or bind the handler function to it.
18 remaining items
I think we have to take it case by case as I'm not sure there's a general rule that would work.
I think we have to take it case by case as I'm not sure there's a general rule that would work.
Can we at least try to outline some kind of rules to refer to? Right now I find it personally very confusing what is expected from me as a library developer and also what I can expect as a user.
We need some form of guideline here...
e.g. IMO it's better to say to users that we don't propagate across events, rather than say that we sometimes propagate across events.
Reacted by Anna Henningsen- added a commit that references this issue
on Aug 6, 2020 To be blunt, I don’t think we can get any kind of reasonable async tracking for event emitters that isn’t implemented by the built-in
EventEmitterclass itself, there’s just too much code written out there that doesn’t expect this.AsyncResource.bindor a possibleasyncResourceoption to theEventEmitterconstructor (which probably wouldn’t even give us the right thing!) are nice but just not practical as actual solutions in the bigger picture here.And I know it’s not great to go and decrease EE performance, but if we end up recommending everybody to do the changes on the consumer side, the only difference is the massive amount of work that needs to be done in the ecosystem.
@addaleax ... it's questionable even if it were built-in to
EventEmitterdue simply to backwards compatibility issues (note that theeventemitter3module on npm has 20 million weekly downloads and would need to be kept in sync). There would, at the very least, need to be a way of gracefully detecting when the async tracking is not supported so that it could be polyfilled, and any module that supports multiple down-level versions of Node.js would still need to handle this in order to propagate context appropriately. If nothing else, I think we'd have to handle this in much the same way ascaptureRejections...initially make it an opt in configuration option that we see later if we can always turn on.Considering what's written above, we could create an
AsyncResourceimplicitly whenee.on()is called. This also assumes that the event listener function will be bound to the outer context. By making this change, we will guarantee that EE is integrated withasync_hooksin an intuitive way (similar to what we have with all other async resources).But the main concern here is a certain performance penalty, as this approach will mean some litter and
AsyncResourceinitialization introduced for every.on()invocation. To mitigate this penalty, we could enable this behavior only when there are active hooks in place. Or maybe someone has a better idea?Reacted by Robert NagyWe needed context propagation on EventEmitters, and we couldn't easily bind all the
eventEmitter.on()calls.In our case, we ended up overriding the
.emitmethod of those event emitters (namely, the event emitters of arequestand of awebsocket) to get a behavior similar to the one from Node.js domains:asyncLocalStorage.run({}, () => { req.emit = AsyncResource.bind(req.emit.bind(req)); /* ... */ });
Reacted by Toni Villena@ronag since you're the OP and this issue has been open for 2.5 years with no movement: do you foresee this happening?
I have no idea. I'm no longer on top of this. Feel free to close.
Okay, I'll go ahead and close it. Holler if it should be reopened.
Would simplify some things and make it more ergonomic to ensure async context propagation in various API's. Though probably with some performance impact?