search

nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
105.69k stars 28.68k forks source link

Async Hooks and Streams #33749

Open ronag opened 4 years ago

ronag commented 4 years ago

Continuing a little bit from https://github.com/nodejs/node/issues/33723.

Do we need closer integration between async hooks and streams? In particular since Stream.destroy can be invoked from basically anywhere the 'close' event can be emitted in a for user unexpected async scope (not sure yet about the correct terminology in async hooks context).

What currently seems to be the way to approach this is to monkey patch destroy after creating a stream, e.g.

const stream = new Duplex(...)
stream.destroy = asyncResource.runInAsyncScope.bind(asyncResource, stream.destroy, stream)

Maybe would make sense to be able to provide a asyncId or asyncTriggerId (not sure of the difference yet) as a constructor argument?

addaleax commented 4 years ago

I think it makes sense to first clarify whether we want to do something for EventEmitters in general, i.e. options 1 or 2 from https://github.com/nodejs/node/issues/33723#issuecomment-638891420. I’ll re-open that issue.

ronag commented 4 years ago

I’ll re-open that issue.

Yes, please.

jasnell commented 4 years ago

EventEmitter is going to be a tricky one if only because of how performance sensitive it is. It absolutely makes sense for an EventEmitter to be tracked as an async resource but when I went through the exercise of making it one it ended up 2x-3x slower in regular use scenarios (streams, http servers, etc). I'm 100% in favor of doing something here but the performance loss problem absolutely needs to be addressed.