otelgin: copy MultipartForm back to original request so net/http cleans it up#8851
Open
SAY-5 wants to merge 1 commit intoopen-telemetry:mainfrom
Open
otelgin: copy MultipartForm back to original request so net/http cleans it up#8851SAY-5 wants to merge 1 commit intoopen-telemetry:mainfrom
SAY-5 wants to merge 1 commit intoopen-telemetry:mainfrom
Conversation
…ns it up Middleware swaps c.Request for a context-carrying copy before running downstream handlers, then restores the original context on the shadow request. net/http's finishRequest only cleans up MultipartForm temp files on the exact *http.Request it handed ServeHTTP, which is still the untouched original - the MultipartForm populated by `c.FormFile` / ParseMultipartForm during the request lives on the shadow request and is never reaped. For any Content-Type: multipart/form-data upload larger than MaxMultipartMemory, the uploaded bytes spill into `multipart-*` temp files that accumulate on disk (open-telemetry#5946). Hold on to the original *http.Request, and in the deferred restore copy c.Request.MultipartForm back onto it before reassigning so finishRequest finds the form to clean up. The spanContext path is unchanged (we already saved its ctx). Added TestMiddlewarePropagatesMultipartFormBack which posts a multipart payload through the middleware and asserts the original request's MultipartForm is non-nil after ServeHTTP returns; the test fails on the old shadow-request code path. Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com> Fixes open-telemetry#5946
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.
Summary
Fixes #5946.
otelgin.Middlewareswapsc.Requestfor a context-carrying copy before running downstream handlers, then restores the original context on the shadow request. But net/http'sfinishRequestonly cleans upMultipartFormtemp files on the exact*http.Requestit handedServeHTTP, which is still the untouched original. TheMultipartFormpopulated byc.FormFile/ParseMultipartFormduring the request lives on the shadow request and is never reaped. For any multipart upload larger thanMaxMultipartMemory, the uploaded bytes spill intomultipart-*temp files that accumulate on disk.Fix
Hold on to the original
*http.Request, and in the deferred restore copyc.Request.MultipartFormback onto it before reassigning, sofinishRequestfinds the form to clean up. The span-context path (savedCtx) is unchanged.Test plan
go build ./...go test ./... -count=1TestMiddlewarePropagatesMultipartFormBack- posts a multipart payload through the middleware and asserts the original request'sMultipartFormis non-nil afterServeHTTPreturns. The test fails on master (shadow-request leak) and passes with this patch.