Open
Conversation
…pool races Add tests to pkg/middleware that reproduce three related bugs in the request coalescing (singleflight) and buffer pool lifecycle: 1. TestSingleflightWinnerWritesBodyTwice: The singleflight winner gets its body written into the CustomWriter buffer by next() inside the callback, then the post-Do code unconditionally writes sfWriter.body back into the same buffer. The buffer reset via defer and slice alias behavior currently masks this for solo requests, but the pattern is unsafe. 2. TestBufferPoolReuseAfterCancellation: When a client cancels a request, ServeHTTP returns and defer s.bufPool.Put(bufPool) returns the buffer to the pool while the orphaned upstream goroutine is still writing to it. A subsequent request can pick up the same buffer. Run with -race to detect the data race on errorCacheCh (defer close races with the goroutine sending on it). 3. TestCoalescedRequestGetsCleanBody: Two concurrent requests coalesced via singleflight both call Upstream which writes sfWriter.body into customWriter.Buf. Run with -race to detect the concurrent bytes.Buffer.Write race. 4. TestCancelledRequestDoesNotCorruptCoalescedResponse: Request A and B are coalesced, A is cancelled mid-flight. Run with -race to detect the errorCacheCh race between the cancelled request's deferred close and the orphaned goroutine. 5. TestBufferBytesSliceCorruption: Unit test demonstrating that bytes.Buffer.Bytes() returns a slice alias (not a copy) that gets corrupted when the buffer is returned to sync.Pool and reused. This is the mechanism underlying the singleflight value body corruption. These tests pass without -race (the races are timing-dependent) but reliably fail with -race, confirming the bugs exist in production code.
Three related bugs caused responses to contain duplicated or corrupted bodies when requests were coalesced via singleflight: 1. Missing Buf.Reset() before writing singleflight result: after singleflight.Do() returns, the post-Do code wrote sfWriter.body into customWriter.Buf without resetting it first, causing the body to be appended a second time. Fixed in both Upstream and Revalidate methods. 2. bytes.Buffer.Bytes() slice alias in singleflight value: the returned singleflightValue captured a slice pointing to the buffer's internal array (not a copy). When the buffer was reset or returned to the pool, shared callers received corrupted data. Fixed by copying the buffer contents into an independent slice before returning. 3. errorCacheCh data race on cancellation: the unbuffered channel with defer close() raced with the goroutine's send when the request context was cancelled. Fixed by using a buffered channel (capacity 1) and removing the defer close. Also fixed the buffer pool lifecycle: the buffer is no longer returned to the pool when the goroutine is still running (on context cancellation).
✅ Deploy Preview for teal-sprinkles-4c7f14 canceled.
|
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
1 participant
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.
Problem
When a client cancels a request that is being coalesced via singleflight, subsequent responses can contain duplicated or corrupted bodies — two HTTP response bodies concatenated together.
Root Cause
Three related bugs in
pkg/middleware/middleware.go:1. Missing
Buf.Reset()before writing singleflight resultAfter
singleflight.Do()returns, the post-Do code writessfWriter.bodyback intocustomWriter.Bufwithout resetting it first. Sincenext()already wrote the body during the Do callback, the body gets appended a second time.This was partially fixed in the Traefik override in f1195d5 but never propagated to core.
2.
bytes.Buffer.Bytes()slice alias corruptionThe singleflight value captures
customWriter.Buf.Bytes(), which returns a slice pointing to the buffer's internal array — not a copy. When the buffer is reset (via defer) or returned tosync.Pooland reused, the shared slice contents are corrupted for coalesced requests still reading it.3.
errorCacheChdata race on cancellationServeHTTPcreates an unbuffered channel and defersclose(). When the client cancels, the deferred close fires while the orphaned goroutine runningUpstreamlater tries to send on the closed channel. Additionally,defer s.bufPool.Put(bufPool)returns the buffer to the pool while the goroutine may still be writing to it.Fix
customWriter.Buf.Reset()before writingsfWriter.bodyin bothUpstreamandRevalidate[]bytebefore returning from the singleflight callbackdefer close()and the recover wrappererrorCacheChreceive), intentionally leak on cancellation so the goroutine can finish safelyTests
5 tests added in the prior commit, covering all three bugs:
TestSingleflightWinnerWritesBodyTwiceTestBufferPoolReuseAfterCancellationerrorCacheChafter cancellationTestCoalescedRequestGetsCleanBodyTestCancelledRequestDoesNotCorruptCoalescedResponseTestBufferBytesSliceCorruptionBuffer.Bytes()slice alias corruption deterministicallyAll pass with
go test -race -count=3.Related