René's URL Explorer Experiment

Title: quic: improve stream cleanup & lookup by pimterry · Pull Request #65944 · nodejs/node · GitHub

Open Graph Title: quic: improve stream cleanup & lookup by pimterry · Pull Request #65944 · nodejs/node

X Title: quic: improve stream cleanup & lookup by pimterry · Pull Request #65944 · nodejs/node

Description: Extracted from #63995, but with a few further improvements en route. The core change here is to stop querying for our Stream instance by id in every on_read_data, on_acked_stream_data and on_receive_data call (all hot paths where this is awkward & expensive). We now cache it in nghttp3's stream-linked data, which it then passes back to each callback for us, so we can just read it directly. Note that both ngtcp2 & nghttp3 separately track per-stream user data, which is a bit confusing for some of the explanation below - we're caching this in nghttp3 user data. Having extracted that though and working through the logic to clean up when H3 streams close, I found some interesting problems, so this expanded a bit: We actually already stashed &stream on every nghttp3 client stream (the last argument to nghttp3_conn_submit_request) but we never read it or never cleaned it up - this was a dangling pointer that's only safe because it was unused. There are multiple cleanup paths required (local destroy & remote close can happen in either order) but the stream closure chain to do that had a series of nested methods which took overlapping parts of the process and weaved in & out of nghttp3/ngtcp2 callbacks unnecessarily en route, with special cases depending on which other steps had already completed. This was hard to follow, but also created some bugs: Applications weren't actually informed about local stream closure, and in some cases nghttp3 never cleaned up local streams at all: stream.destroy() from JS called Session::RemoveStream which cleared ngtcp2 stream user data beforehand, and then Session::on_stream_close exited early if the ngtcp2 user data was null. That means we skipped calling ReceiveStreamClose, which means we never called nghttp3_conn_close_stream2, which is what actually frees the nghttp3 stream. End result is that locally destroyed nghttp3 streams were orphaned & retained for the lifetime of the entire connection. Http3ApplicationImpl::OnStreamClose called ExtendMaxStreams, and then Session::RemoveStream called ngtcp2_conn_extend_max_streams_* as well, so closing any remote stream actually expanded the max-streams window twice. Http3ApplicationImpl::OnStreamClose called ExtendMaxStreams with REMOTE unconditionally, even though it was called for local streams too, so closing a local stream expanded the remote stream window as well. More generally: the max stream window was tightly tied to the lifetime of our Stream object & application behaviour, not the actual live streams in ngtcp2, which is the thing it should be limiting. End result: we had a few different & overlapping paths that stream closure could take, and the interactions created a selection of small bugs. Src changes here are a net deletion, simplifying this and fixing the bugs: There's now one hook for "stream on the wire closed" which runs in all cases when a QUIC stream is actually closed. Session:on_stream_close calls application().ReceiveStreamClose(), and the application cleans up its state there in one place. There's now one hook for "local Stream destruction" (by JS, or after closure on the wire): Session::RemoveStream cleans up that side, and calls application.StreamRemoved() to inform the app when it's gone. Nghttp3 stream close callbacks become redundant, so that code is just removed. Nghttp3 close events only ever happened after we told nghttp3 about it from ReceiveStreamClose, so we just cleanup in a single step there instead. The max-stream limit is tied directly to ngtcp2 stream closure: when on_stream_close fires, the credit is returned immediately. Max-stream logic on Applications (ExtendMaxStreams, EndpointLabel) goes away completely. The original goal: we cache our Stream object in nghttp3 so we don't have to look it up for every data callback, and successfully clean that up later when the Stream is gone.

Open Graph Description: Extracted from #63995, but with a few further improvements en route. The core change here is to stop querying for our Stream instance by id in every on_read_data, on_acked_stream_data and on_receiv...

X Description: Extracted from #63995, but with a few further improvements en route. The core change here is to stop querying for our Stream instance by id in every on_read_data, on_acked_stream_data and on_receiv...

Opengraph URL: https://github.com/nodejs/node/pull/65944

X: @github

direct link

Domain: github.com

Links:

Viewport: width=device-width