Repository navigation
Fix h2 close race - #580
Open
sushantshankar2414 wants to merge 10 commits into
Open
sushantshankar2414 wants to merge 10 commits into
sushantshankar2414 wants to merge 10 commits into
Conversation
Closes coder#435 When compression was enabled, `Conn.Write` sent messages across many small frames due to the flate library's internal `bufferFlushSize` (240 bytes). Each flush triggered a `writeFrame` call, producing alternating ~236 and 4 byte frames. `Conn.Write` now compresses the entire message into a buffer first, then transmits it as a single frame. Messages below `flateThreshold` bypass compression and are sent uncompressed in a single frame.
… read (coder#565) read: avoid per-frame cleanup closures Every frame header and payload read called prepareRead, which returned a cleanup function to clear the timeout and translate close or cancellation errors. That function captured the context, connection, and the address of the caller's named error result. Because prepareRead returned the function, the closure outlived its stack frame and Go allocated its captured state on the heap for every frame read. Move the cleanup logic to a normal finishRead method and defer a direct method call instead. This preserves timeout cleanup and error translation without returning a closure. Compiler escape analysis with -gcflags=-m=2 confirms that the old function literal escaped and forced the named error result in readFrameHeader and readFramePayload onto the heap; neither escape remains after this change. The results below compare parent d099e16 with this commit on an Apple M4 Max using GOMAXPROCS=1 and benchstat over 10 samples. A temporary in-package harness, not included in this commit, repeatedly called one internal frame read. Header reads parse a minimal frame header; payload reads copy 512 bytes from a buffered repeating reader. The background cases use context.Background, while the cancelable cases use an uncanceled context.WithCancel. Removing the escaping closure eliminates 2 allocations and 64 bytes from every frame read: one allocation for the closure environment and one for the named error result retained by that closure. Header reads with a background context improve from 170.5 to 118.5 ns/op, 216 to 152 B/op, and 6 to 4 allocs/op. Header reads with a cancelable context improve from 211.1 to 158.7 ns/op with the same allocation reduction. Payload reads remove the same fixed overhead; they remain slower because the benchmark also copies 512 bytes. Both context types benefit because this commit does not change timeout registration. It only removes cleanup allocations made after every prepareRead call. An interleaved 12-sample BenchmarkConn/disabledCompress run, which exercises complete message reads, improves from 3.875 to 3.657 us/op (-5.63%), 42 to 32 allocs/op (-23.81%), and 1536 to 1216 B/op (-20.83%).
Every frame read and write registered a context.AfterFunc callback so a context cancellation could close the connection. context.Background cannot be canceled because its Done channel is nil, but the code still constructed, stored, stopped, and cleared a callback registration for each operation. Skip that work when ctx.Done() is nil, and only clear a timeout when one was actually installed. BenchmarkConn also joins its writer goroutine after timing so repeated benchmark runs finish cleanly. The results below compare parent 7039364 with this commit on an Apple M4 Max using GOMAXPROCS=1 over 10 samples. A temporary in-package harness, not included in this commit, repeatedly called one internal frame read using either context.Background or an uncanceled context.WithCancel. Header reads parse a minimal frame header; payload reads copy 512 bytes from a buffered repeating reader. Before this change, setupReadTimeout called context.AfterFunc even for context.Background. Avoiding that dead registration reduces background header reads from 121.95 to 29.94 ns/op and payload reads from 124.17 to 29.15 ns/op. Both fall from 152 B/op and 4 allocs/op to zero. Cancelable contexts still need an AfterFunc registration, so they remain at 152 B/op and 4 allocs/op. BenchmarkConn/disabledCompress uses a cancelable context as well, so it remains at 1219 B/op and 32 allocs/op.
Close can run on a context.AfterFunc goroutine (e.g. after the peer sends RST_STREAM) while the handler goroutine is in Write. Both touched the same http.ResponseWriter, which is not safe for concurrent use. Write already flushes after every write, so the Flush in Close was unnecessary.
sushantshankar2414
marked this pull request as draft
October 22, 2026 14:02
sushantshankar2414
marked this pull request as ready for review
October 22, 2026 14:02
This branch has not been deployed
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.
Tested on go1.26.4 linux/amd64 (x/net v0.44.0).
go test -race -count=50 -run TestHTTP2Suite_XCONNECT_Enabledininternal/thirdparty/http2:
before: 19/50 runs fail with a data race (seen on both TLS and h2c)
after: 0/50 (fix below)
Cause: the client's deferred CloseNow resets the stream (RST_STREAM),
x/net cancels r.Context(), and the setupReadTimeout AfterFunc calls
Conn.close → h2ServerStream.Close → Flush on its own goroutine. At the
same time the handler's deferred Conn.Close writes a close frame via
h2ServerStream.Write → Flush. Both use the same http.ResponseWriter,
which isn't safe for concurrent use (unlike net.Conn on the HTTP/1.1
path). Trace: [gist link]
This also answers the TODO in h2ServerStream.Close: the flush isn't
needed, since Write already flushes after every write.
With this change
go test -race ./...passes in the root module andinternal/thirdparty. Happy to open a PR against this branch if that's
easier.
Possible follow-up: on HTTP/1.1, rwc.Close() unblocks a stuck Write,
which setupWriteTimeout relies on. On HTTP/2, closing the request body
doesn't unblock a ResponseWriter.Write waiting on flow control, so
write timeouts may not fire when the peer stops reading. Happy to
write a test for that.