Skip to content
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

Async Hooks and Streams #33749

Open
ronag opened this issue Jun 5, 2020 · 3 comments
Open

Async Hooks and Streams #33749

ronag opened this issue Jun 5, 2020 · 3 comments
Labels
async_hooks Issues and PRs related to the async hooks subsystem. stream Issues and PRs related to the stream subsystem.

Comments

@ronag
Copy link
Member

ronag commented Jun 5, 2020

Continuing a little bit from #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?

@ronag ronag added stream Issues and PRs related to the stream subsystem. async_hooks Issues and PRs related to the async hooks subsystem. labels Jun 5, 2020
@addaleax
Copy link
Member

addaleax commented Jun 5, 2020

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 #33723 (comment). I’ll re-open that issue.

@ronag
Copy link
Member Author

ronag commented Jun 5, 2020

I’ll re-open that issue.

Yes, please.

@jasnell
Copy link
Member

jasnell commented Jun 5, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants