Repository navigation
Fix h2 close race - #580
Open
sushantshankar2414 wants to merge 6 commits into
Open
sushantshankar2414 wants to merge 6 commits into
sushantshankar2414 wants to merge 6 commits into
Conversation
- Fix 'exampels' typo in CI workflow comments - Fix README paths to use 'internal/examples/http2' - Change visibleAddr to display 0.0.0.0 for all-interface bindings
Rename the Protocol type to HTTPProtocol for consistency with existing HTTPClient and HTTPHeader field naming in DialOptions. Constants are also renamed: - ProtocolHTTP1 → HTTPProtocol1 - ProtocolHTTP2 → HTTPProtocol2 - ProtocolAcceptAny → HTTPProtocolAny
Simplify the TLS example by requiring -cert and -key flags instead of auto-generating a self-signed certificate. This reduces complexity and keeps the example focused on HTTP/2 WebSocket usage.
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
mafredri
force-pushed
the
mafredri/http2
branch
from
October 29, 2026 21:32
3f2f9af to
fd5e7f7
Compare
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.