test: add tests reproducing singleflight body duplication and buffer pool races#703
Open
cdaguerre wants to merge 1 commit intodarkweak:masterfrom
Open
test: add tests reproducing singleflight body duplication and buffer pool races#703cdaguerre wants to merge 1 commit intodarkweak:masterfrom
cdaguerre wants to merge 1 commit intodarkweak:masterfrom
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.
✅ 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. This manifests as responses containing two HTTP bodies concatenated together.
Root Cause
Three related bugs in
pkg/middleware/middleware.go:Missing
Buf.Reset()before writing singleflight result — Aftersingleflight.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.)bytes.Buffer.Bytes()slice alias corruption — The singleflight value capturescustomWriter.Buf.Bytes(), which returns a slice pointing to the buffer's internal array, not a copy. When the buffer is returned tosync.Pooland reused, the shared slice contents are corrupted for coalesced requests still reading it.errorCacheChdata race on cancellation —ServeHTTPcreates 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, causing a race (and potential panic caught by recover).This PR
Adds 5 tests that reproduce these bugs:
-raceTestSingleflightWinnerWritesBodyTwiceTestBufferPoolReuseAfterCancellationTestCoalescedRequestGetsCleanBodyTestCancelledRequestDoesNotCorruptCoalescedResponseTestBufferBytesSliceCorruptionRun with
go test -race -count=1 -v ./pkg/middleware/to see the races.Related