[tailscale] net/http/internal/http2: let request-write goroutines exit early - #173
Open
bradfitz wants to merge 1 commit into
Open
[tailscale] net/http/internal/http2: let request-write goroutines exit early#173bradfitz wants to merge 1 commit into
bradfitz wants to merge 1 commit into
Conversation
…t early The goroutine spawned per RoundTrip to write the request previously parked until the stream ended, even after the request was fully sent, just to wait for the stream-end events and run cleanupWriteRequest. For clients with many concurrent long-lived response streams (long polls, event streams), that's a parked goroutine and its stack per stream doing nothing, which adds up to a large fraction of such a client's memory use. Once the request is fully sent, detach: the goroutine exits, and cleanupWriteRequest instead runs (on a short-lived goroutine) from whichever stream-end event fires first. Both stream-end events (peer half-close and abort) are raised under cc.mu, so a mutex-guarded flag gives exactly-once cleanup dispatch. Request context cancellation is watched via context.AfterFunc, and ResponseHeaderTimeout is enforced by a time.AfterFunc timer armed at detach and disarmed when response headers arrive. Only the deprecated Request.Cancel channel still requires a parked goroutine to watch it, so requests using it keep the previous behavior. Intended for upstreaming; carried in the Tailscale fork until then. Updates tailscale/corp#29053 Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com> Change-Id: I5500c691194457195b0869c1612bf93718ca96c1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The goroutine spawned per RoundTrip to write the request previously
parked until the stream ended, even after the request was fully sent,
just to wait for the stream-end events and run cleanupWriteRequest.
For clients with many concurrent long-lived response streams (long
polls, event streams), that's a parked goroutine and its stack per
stream doing nothing, which adds up to a large fraction of such a
client's memory use.
Once the request is fully sent, detach: the goroutine exits, and
cleanupWriteRequest instead runs (on a short-lived goroutine) from
whichever stream-end event fires first. Both stream-end events (peer
half-close and abort) are raised under cc.mu, so a mutex-guarded flag
gives exactly-once cleanup dispatch. Request context cancellation is
watched via context.AfterFunc, and ResponseHeaderTimeout is enforced
by a time.AfterFunc timer armed at detach and disarmed when response
headers arrive. Only the deprecated Request.Cancel channel still
requires a parked goroutine to watch it, so requests using it keep
the previous behavior.
Intended for upstreaming; carried in the Tailscale fork until then.
Updates tailscale/corp#29053