Repository navigation
net/http: cherry-pick http2 serveConn goroutine removal PRs - #187
Merged
Merged
Conversation
…e goroutine exit when idle Implement the TODO from 2014 at the top of server.go: a server connection with nothing to do used to pin a goroutine blocked in the serve loop (reached through net/http's conn.serve and ServeConn) in addition to the readFrames goroutine; now it pins only readFrames. When the serve loop has nothing runnable (nothing being written, unflushed, or poppable on the write side, and no shutdown in progress), the serve goroutine parks: it simply exits. Every sender into the serve loop's channels first calls beginServeSend, which counts in-flight sends and revives a parked loop on a new goroutine, so a frame from the peer, a handler write or body read, a timer firing, or a graceful shutdown request wakes the connection back up. Open streams and running handlers don't prevent parking, since the handler-side paths into the serve loop (response writes, body reads, stream deadline timers, pushes) all go through beginServeSend too. This helps SSE/long-poll-style servers: a connection whose handler is parked mid-long-poll for hours pins only the readFrames and handler goroutines, and the handler's next Write or Flush revives the loop. With open streams, the write scheduler can still hold DATA frames blocked on flow control; that's fine to park on, because only an incoming WINDOW_UPDATE or SETTINGS frame can unblock them, and incoming frames revive the loop. The serve loop's teardown became explicit rather than deferred, and it takes over the cleanup that ServeConn's callers used to do after it returned (closing the connection, unregistering it, and running net/http's StateClosed hook), signaled through the new ServeConnOpts.OnClose hook. Callers that don't set OnClose keep the old blocking ServeConn semantics and never park. Parking happens between the frames of an active request, so the park/resume cycle runs on hot paths. Spawning the resume goroutine with a pre-allocated method-value closure keeps it allocation-free: BenchmarkClientServer/h2 reports the same 68 allocs/op as before, with ns/op unchanged within noise. As a temporary safety measure, the GODEBUG=http2serveparking=0 setting disables parking, restoring the old behavior of keeping a goroutine parked for the lifetime of each connection. The setting is undocumented (#-prefixed), so it is not listed in doc/godebug.md or runtime/metrics. This is the server-side counterpart of CL 810780, which let the Transport's request-write goroutine exit early for the same reason: for servers with many mostly-idle connections, a parked goroutine and its stack per connection add up. Cherry-pick notes: the only conflict was in the serverConn struct field list, where this tree still has the srv field that upstream had removed and this change re-adds; the upstream field list was taken. The doc/next release note was dropped, since the 1.27 release branch has no doc/next directory. Updates golang#80735 Updates golang#81524 Updates #174 Change-Id: I4a9fd4e2ce7a7a4e0f84179e54a9cc97d1a62ad5 Reviewed-on: https://go-review.googlesource.com/c/go/+/831084 Reviewed-by: Nicholas Husin <husin@google.com> Reviewed-by: Nicholas Husin <nsh@golang.org> LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Damien Neil <dneil@google.com> (cherry picked from commit 5c51011)
… serve parking change Fix a rebase comment mishap: the doc comment describing canPark was attached to the http2serveparking var, leaving canPark undocumented. Skip the parkMu accounting in beginServeSend and endServeSend on connections that can never park (no OnClose hook, or parking disabled with GODEBUG=http2serveparking=0), removing a mutex round trip per frame on both the sender and receiver side. Add a test that a response blocked on the peer's flow control window parks the serve goroutine with its DATA queued in the write scheduler, and that a WINDOW_UPDATE revives it. Updates golang#80735 Updates golang#81524 Updates #174 Change-Id: Ic1a3f7b9d2e5a4c8f6b0d7a9e3c5f2b8d1a6e4c7 Reviewed-on: https://go-review.googlesource.com/c/go/+/832504 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Damien Neil <dneil@google.com> Reviewed-by: Nicholas Husin <husin@google.com> Reviewed-by: Nicholas Husin <nsh@golang.org> (cherry picked from commit 0ac31b6)
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
1 participant
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.
Uh oh!
There was an error while loading. Please reload this page.