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

Does autoDestroy make sense for http2 streams? #24576

Open
addaleax opened this issue Nov 23, 2018 · 4 comments
Open

Does autoDestroy make sense for http2 streams? #24576

addaleax opened this issue Nov 23, 2018 · 4 comments
Labels
http2 Issues or PRs related to the http2 subsystem. question Issues that look for answers.

Comments

@addaleax
Copy link
Member

  • Subsystem: http2

Now that we have autoDestroy support in the streams implementation, I think we would like to move streams towards using that option if possible. I have a hard time figuring out if this makes sense for HTTP/2 streams? (i.e., should we destroy streams once both readable and writable side are finished?)

I have a hard time understanding HTTP/2 stream’s lifecycle behaviour, to be honest, and I’d appreciate any explanations about it :)

/cc @nodejs/http2 @mcollina

@addaleax addaleax added http2 Issues or PRs related to the http2 subsystem. question Issues that look for answers. labels Nov 23, 2018
@mcollina
Copy link
Member

autoDestroy solves a specific problem in the stream machinery: it ensures that whenever there is an emit('error') in the codebase, the stream actually get destroyed properly and 'close'  is emitted. This is the source of a lot of bugs, especially in net and fs.
'http2' has been developed under those principles already, so I would update it as the last one. The HTTP/2 lifecycle has been extremely complicated because of the lack of autoDestroy: true, and it needs to work around the lack of it.

As an example, consider this code in fs https://github.com/nodejs/node/blob/master/lib/internal/fs/streams.js#L116-L120. Basically we are waiting for 'end' to fire to tear down things. autoDestroy: true makes that part of the stream machinery.

These are my 2 cents. This topic is extremely complex.

@sebdeckers
Copy link
Contributor

TIL ... Any docs on the autoDestroy property?

@Fishrock123
Copy link
Member

@sebdeckers Please search the docs: https://nodejs.org/dist/latest-v11.x/docs/api/stream.html

@BridgeAR
Copy link
Member

BridgeAR commented Jan 4, 2020

@ronag this might be interesting for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem. question Issues that look for answers.
Projects
None yet
Development

No branches or pull requests

5 participants