Skip to content

[3.5] cherry-pick rpc: fix nil-pointer panic in gzip batch flush race (#22338)#22383

Merged
yperbasis merged 2 commits into
release/3.5from
lupin012/fix_batch_gzip_crash
Jul 13, 2026
Merged

[3.5] cherry-pick rpc: fix nil-pointer panic in gzip batch flush race (#22338)#22383
yperbasis merged 2 commits into
release/3.5from
lupin012/fix_batch_gzip_crash

Conversation

@lupin012

Copy link
Copy Markdown
Contributor

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).Flushgzip.(*Writer).FlushgzipResponseWriter.FlushrunMethodhandleCallhandleCallMsghandleBatch).
  • 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

  • go test -race -run TestGzipHandlerBatchConcurrentStreamableFlush ./node/...
  • go test -race -count=1 ./rpc/... ./node/...
  • make lint
  • 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.

lupin012 added 2 commits July 10, 2026 14:46
## 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.
@lupin012 lupin012 marked this pull request as ready for review July 11, 2026 06:13
@lupin012 lupin012 added the RPC label Jul 11, 2026
@AskAlexSharov AskAlexSharov requested a review from Copilot July 12, 2026 13:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: WithGzipStreamingHook panics on nil hook; runMethod guards against typed-nil func() 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 thread rpc/http.go
Comment on lines +264 to +266
func withoutGzipStreamingHook(ctx context.Context) context.Context {
return context.WithValue(ctx, httpFlusherContextKey{}, nil)
}
@yperbasis yperbasis merged commit a2143ba into release/3.5 Jul 13, 2026
166 checks passed
@yperbasis yperbasis deleted the lupin012/fix_batch_gzip_crash branch July 13, 2026 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants