[3.5] cherry-pick rpc: fix nil-pointer panic in gzip batch flush race (#22338)#22383
Merged
Conversation
## Summary A JSON-RPC batch containing 2+ streamable methods ran each call on its own goroutine, and each one invoked the shared gzip-streaming flush hook installed on the request context. `gzipResponseWriter.Flush` is not safe for concurrent use, so calling it from multiple goroutines raced on the underlying `gzip.Writer` and could dereference a nil flate compressor, crashing the node. Batch sub-calls write into private per-item buffers rather than the HTTP response writer, so the hook was never useful there in the first place. Mask it out of the context used by batch sub-call goroutines instead of hardening `gzipResponseWriter` itself, since it remains owned by a single goroutine everywhere else. Also hardens the hook mechanism itself: `WithGzipStreamingHook` now panics if ever called with a nil hook, and `runMethod` guards the flush call with `ok && flush != nil`, so the mechanism doesn't rely on the hook always being stored as an untyped nil to stay safe. Fixes #22334 ## Tests - Added `node/rpcstack_gzip_batch_race_test.go` (`TestGzipHandlerBatchConcurrentStreamableFlush`): sends a real JSON-RPC batch of 8 calls to a streamable method through the actual gzip middleware and `rpc.Server`, with `Accept-Encoding: gzip`, and each call's payload sized above `minGzipBodySize` so the fixed path still gzips. Asserts `HTTP 200`, `Content-Encoding: gzip`, and that the decoded batch contains all `n` responses with the expected ids/results — not just reliance on `-race` to catch a regression. Before the fix, running it with `-race` reliably reproduced both the data race and, on some runs, the exact nil-pointer panic reported in the issue. After the fix it passes cleanly and repeatably under `-race` (verified over 15 consecutive runs). - Added `rpc/http_test.go` (`TestWithGzipStreamingHookPanicsOnNilHook`): pins that `WithGzipStreamingHook` panics when given a nil hook. - Added `rpc/handler_test.go` (`TestRunMethodFlushHookNilFunc`): pins that `runMethod` does not panic when the hook stored on the context is a typed nil `func()` rather than an untyped nil. - Full `rpc/...` and `node/...` suites pass with `-race`. ### Manual end-to-end verification Also reproduced the issue against two real running nodes, one built from this branch's parent commit and one from this branch, both sent the exact same JSON-RPC batch (16 calls to `debug_traceBlockByNumber`) with `Accept-Encoding: gzip`: - **Pre-fix binary**: crashed on the first request, with a panic identical to the one reported in the issue (`flate.(*Writer).Flush` → `gzip.(*Writer).Flush` → `gzipResponseWriter.Flush` → `runMethod` → `handleCall` → `handleCallMsg` → `handleBatch`). - **Fixed binary**: the same request succeeded (`HTTP 200`, correct JSON body) across 8 consecutive attempts, node stayed up and kept producing blocks throughout. ## Test plan - [x] `go test -race -run TestGzipHandlerBatchConcurrentStreamableFlush ./node/...` - [x] `go test -race -count=1 ./rpc/... ./node/...` - [x] `make lint` - [x] Manual repro against a real node: pre-fix binary crashes, fixed binary doesn't ## Note A pre-existing deadlock in `handleBatch` (`wg.Add(len(msgs))` vs. `len(calls)` goroutines spawned) was flagged during review. It's unrelated to the gzip race fixed here and will be addressed in a separate PR.
newTestRPCServer was added on main by the httpServer refactor (#21724), which release/3.5 doesn't have, so the cherry-picked test failed to compile here. Construct the rpc.Server directly instead.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a concurrency-driven nil-pointer panic when serving gzipped JSON-RPC batch requests containing multiple streamable methods by preventing batch sub-call goroutines from invoking the shared gzip streaming flush hook on the HTTP response writer.
Changes:
- Mask the gzip-streaming flush hook from the context used by batch sub-call goroutines to prevent concurrent
Flush()on the shared gzip writer. - Harden the gzip-streaming hook mechanism:
WithGzipStreamingHookpanics on nil hook;runMethodguards against typed-nilfunc()hooks. - Add regression tests covering the batch gzip race scenario and hook invariants.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
rpc/http.go |
Adds nil-hook panic guard and introduces hook-masking helper for batch sub-calls. |
rpc/http_test.go |
Adds test asserting WithGzipStreamingHook panics on nil hook. |
rpc/handler.go |
Masks gzip streaming hook in batch call processing; guards runMethod flush invocation against typed-nil hooks. |
rpc/handler_test.go |
Adds test ensuring runMethod doesn’t panic when context contains a typed-nil flush hook. |
node/rpcstack_gzip_batch_race_test.go |
Adds end-to-end regression test reproducing the gzipped batch + streamable concurrency scenario. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+264
to
+266
| func withoutGzipStreamingHook(ctx context.Context) context.Context { | ||
| return context.WithValue(ctx, httpFlusherContextKey{}, nil) | ||
| } |
yperbasis
approved these changes
Jul 13, 2026
This was referenced Jul 13, 2026
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.
Summary
A JSON-RPC batch containing 2+ streamable methods ran each call on its own goroutine, and each one invoked the shared gzip-streaming flush hook installed on the request context.
gzipResponseWriter.Flushis not safe for concurrent use, so calling it from multiple goroutines raced on the underlyinggzip.Writerand could dereference a nil flate compressor, crashing the node.Batch sub-calls write into private per-item buffers rather than the HTTP response writer, so the hook was never useful there in the first place. Mask it out of the context used by batch sub-call goroutines instead of hardening
gzipResponseWriteritself, since it remains owned by a single goroutine everywhere else.Also hardens the hook mechanism itself:
WithGzipStreamingHooknow panics if ever called with a nil hook, andrunMethodguards the flush call withok && flush != nil, so the mechanism doesn't rely on the hook always being stored as an untyped nil to stay safe.Fixes #22334
Tests
node/rpcstack_gzip_batch_race_test.go(TestGzipHandlerBatchConcurrentStreamableFlush): sends a real JSON-RPC batch of 8 calls to a streamable method through the actual gzip middleware andrpc.Server, withAccept-Encoding: gzip, and each call's payload sized aboveminGzipBodySizeso the fixed path still gzips. AssertsHTTP 200,Content-Encoding: gzip, and that the decoded batch contains allnresponses with the expected ids/results — not just reliance on-raceto catch a regression. Before the fix, running it with-racereliably reproduced both the data race and, on some runs, the exact nil-pointer panic reported in the issue. After the fix it passes cleanly and repeatably under-race(verified over 15 consecutive runs).rpc/http_test.go(TestWithGzipStreamingHookPanicsOnNilHook): pins thatWithGzipStreamingHookpanics when given a nil hook.rpc/handler_test.go(TestRunMethodFlushHookNilFunc): pins thatrunMethoddoes not panic when the hook stored on the context is a typed nilfunc()rather than an untyped nil.rpc/...andnode/...suites pass with-race.Manual end-to-end verification
Also reproduced the issue against two real running nodes, one built from this branch's parent commit and one from this branch, both sent the exact same JSON-RPC batch (16 calls to
debug_traceBlockByNumber) withAccept-Encoding: gzip:flate.(*Writer).Flush→gzip.(*Writer).Flush→gzipResponseWriter.Flush→runMethod→handleCall→handleCallMsg→handleBatch).HTTP 200, correct JSON body) across 8 consecutive attempts, node stayed up and kept producing blocks throughout.Test plan
go test -race -run TestGzipHandlerBatchConcurrentStreamableFlush ./node/...go test -race -count=1 ./rpc/... ./node/...make lintNote
A pre-existing deadlock in
handleBatch(wg.Add(len(msgs))vs.len(calls)goroutines spawned) was flagged during review. It's unrelated to the gzip race fixed here and will be addressed in a separate PR.