refactor(server): serve unencrypted HTTP/2 natively - #1818
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughServeConnect replaces custom h2c wiring with native protocol configuration and adds tests for HTTP/1.1, unencrypted HTTP/2, and graceful draining of in-flight HTTP/2 requests. ChangesServeConnect HTTP/2 support
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c6d582c-6c4b-4b6a-a088-42e83651274b
📒 Files selected for processing (2)
pkg/server/server.gopkg/server/server_test.go
Replaces the x/net h2c wrapper with the http.Server Protocols field. The server now tracks HTTP/2 connections itself, so Shutdown drains them like HTTP/1.1 ones and the GOAWAY workaround is no longer needed. Cleartext HTTP/2 is negotiated by prior knowledge only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9b76974 to
ed31d55
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/server/server_test.go (1)
83-90: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the in-flight shutdown barrier deterministic.
A scheduler delay exceeding 300ms can let
/slowfinish beforecancel(), so this test may pass without proving that shutdown waits for an active HTTP/2 request. Gate handler completion with a channel, assertwg.Wait()is still blocked afterServereturns, then release it.Proposed fix
entered := make(chan struct{}) +allowFinish := make(chan struct{}) requestDone := make(chan struct{}) ... close(entered) - time.Sleep(300 * time.Millisecond) + <-allowFinish close(requestDone) ... <-entered cancel() <-serveReturned -wg.Wait() +shutdownDone := make(chan struct{}) +go func() { + wg.Wait() + close(shutdownDone) +}() +select { +case <-shutdownDone: + close(allowFinish) + t.Fatal("shutdown completed before the in-flight request finished") +default: +} +close(allowFinish) +<-shutdownDoneAlso applies to: 131-145
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a585d945-4257-49f0-a8b3-11f9cdabc222
📒 Files selected for processing (2)
pkg/server/server.gopkg/server/server_test.go
Coverage Report for CI Build 30522242418Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.01%) to 46.99%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
3 participants
Summary
Replaces the deprecated
golang.org/x/net/http2/h2cwrapper with thehttp.ServerProtocols field (native since Go 1.24). The server serves HTTP/1.1 and unencrypted HTTP/2 itself and tracks the HTTP/2 connections, so graceful shutdown drains them the same way as HTTP/1.1 ones — thehttp2.ConfigureServerGOAWAY workaround (golang/go#26682) is removed.Changes
Protocols(HTTP1, HTTP2, UnencryptedHTTP2) on the connect server; droph2c.NewHandlerand thehttp2.ConfigureServerhookcurl --http2-prior-knowledgeTestGracefulShutdownDrainsInflightHTTP2Requests(an in-flight HTTP/2 request survives shutdown to completion) andTestServeConnectServesBothProtocols(the real ServeConnect answers HTTP/1.1 and HTTP/2.0)x/net/http2is no longer imported by first-party code; the go.mod annotation cleanup rides in a follow-upTechnical Details
Verified against a running server: plain request → HTTP/1.1 200;
curl --http2(Upgrade dance) → HTTP/1.1 200 with the upgrade quietly declined;curl --http2-prior-knowledge→ HTTP/2 200; grpcurl reflection works over plaintext HTTP/2; SIGTERM logs a complete graceful shutdown. HTTP/1.1 clients are unaffected.Test Plan
go build ./...andgo vet ./...passmake lintpasses (0 issues)make testpasses (race detector, -count 2)make e2e-testpasses against live Postgres + SpiceDB containers🤖 Generated with Claude Code