nodejs / node

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

Does `autoDestroy` make sense for http2 streams? #24576

Open addaleax opened 5 years ago

addaleax commented 5 years ago

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

mcollina commented 5 years ago

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 commented 5 years ago

TIL ... Any docs on the autoDestroy property?

Fishrock123 commented 5 years ago

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

BridgeAR commented 4 years ago

@ronag this might be interesting for you.