fix(otelgin): remove multipartform temporary file #6609

Yiling-J · 2025-01-13T13:45:43Z

Fix #5946

linux-foundation-easycla · 2025-01-13T13:45:52Z

The committers listed above are authorized under a signed CLA.

✅ login: Yiling-J (6875165, a87ae01, bfa19b5, 65ce788, 61b90e2, 3131248, 39c3fc5)

codecov · 2025-01-13T14:15:46Z

Codecov Report

Attention: Patch coverage is 75.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 68.6%. Comparing base (9640704) to head (65ce788).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
...ation/github.com/gin-gonic/gin/otelgin/gintrace.go	75.0%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #6609     +/-   ##
=======================================
+ Coverage   68.5%   68.6%   +0.1%     
=======================================
  Files        200     200             
  Lines      16781   16882    +101     
=======================================
+ Hits       11500   11589     +89     
- Misses      4935    4943      +8     
- Partials     346     350      +4

Files with missing lines	Coverage Δ
...ation/github.com/gin-gonic/gin/otelgin/gintrace.go	`87.8% <75.0%> (-2.1%)`	⬇️

... and 2 files with indirect coverage changes

flc1125 · 2025-01-13T14:56:36Z

instrumentation/github.com/gin-gonic/gin/otelgin/test/gintrace_test.go

+			_ = c.AbortWithError(http.StatusInternalServerError, err)
+			return
+		}
+		_ = ff


I believe the content of ff can be detected here.

The goal of the test is to ensure that temporary files are automatically removed, not to test multipart file parsing. The sole purpose of using c.FormFile is to trigger multipart parsing, which creates temporary files. I added a fileHeader.Open() test in this function to confirm that the temporary file is available. However, I think checking the value of ff is unnecessary.

instrumentation/github.com/gin-gonic/gin/otelgin/test/gintrace_test.go

flc1125 · 2025-01-13T15:02:32Z

instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go

+			//
+			// This means that when we are on returning path from handler middlewares up in chain from this middleware
+			// can not access these temporary files anymore because we deleted them here.
+			if c.Request.MultipartForm != nil {


I'm wondering, if there's another middleware layer outside this one, would it be removed because of this?

It's hard to say, but in all cases, regardless of how many middlewares are chained, temporary files should be removed. I didn't check the return error of RemoveAll here, which aligns with the behavior in both Go's http package and echo-contrib. I believe each middleware should independently attempt to remove temporary files.

see: #6609 (comment)

flc1125 · 2025-01-13T16:19:36Z

I have written 2 examples to facilitate the analysis of this issue.

I believe that gin falls into the first category, so I think the main logic of the modifications in this PR should be fine.

Here I am using a URL to simulate the behavior of MultipartForm, and I believe their logic should be consistent.

The first example:

package gin

import (
	"context"
	"net/http"
	"net/url"
	"testing"

	"github.com/stretchr/testify/assert"
)

type handler func(t *testing.T, req *http.Request)

var test1Handler handler = func(t *testing.T, req *http.Request) {
	t.Logf("req1.0: %p, req.URL: %p", req, req.URL)
}

var test2Handler handler = func(t *testing.T, req *http.Request) {
	t.Logf("req2.0: %p, req.URL: %p", req, req.URL)

	saveCtx := req.Context()
	defer func() {
		t.Logf("req2.3: %p, req.URL: %p", req, req.URL)
		req = req.WithContext(saveCtx)
		t.Logf("req2.4: %p, req.URL: %p", req, req.URL)
	}()

	defer func() {
		t.Logf("req2.1: %p, req.URL: %p", req, req.URL)
		req.URL = &url.URL{
			Scheme: "https",
		}
		t.Logf("req2.2: %p, req.URL: %p", req, req.URL)
	}()

	req = req.WithContext(context.WithValue(req.Context(), "key", "value"))
}

var test3Handler handler = func(t *testing.T, req *http.Request) {
	t.Logf("req3.0: %p, req.URL: %p", req, req.URL)
	t.Logf("req3.1: %v", req.Context().Value("key"))
}

func TestIssue_Middleware(t *testing.T) {
	req, err := http.NewRequest("POST", "/upload", nil)
	assert.NoError(t, err)
	req.URL = &url.URL{
		Scheme: "http",
	}

	t.Logf("req0.0: %p, req.URL: %p", req, req.URL)

	handlers := []handler{
		test1Handler,
		test2Handler,
		test3Handler,
	}
	for _, h := range handlers {
		h(t, req)
	}
}

output:

=== RUN   TestIssue_Middleware
    issue_test.go:51: req0.0: 0x1400049ba40, req.URL: 0x1400016ca20
    issue_test.go:15: req1.0: 0x1400049ba40, req.URL: 0x1400016ca20
    issue_test.go:19: req2.0: 0x1400049ba40, req.URL: 0x1400016ca20
    issue_test.go:29: req2.1: 0x1400049bb80, req.URL: 0x1400016ca20
    issue_test.go:33: req2.2: 0x1400049bb80, req.URL: 0x1400016cab0
    issue_test.go:23: req2.3: 0x1400049bb80, req.URL: 0x1400016cab0
    issue_test.go:25: req2.4: 0x1400049bcc0, req.URL: 0x1400016cab0
    issue_test.go:40: req3.0: 0x1400049ba40, req.URL: 0x1400016ca20
    issue_test.go:41: req3.1: <nil>
--- PASS: TestIssue_Middleware (0.00s)

The secend example:

package gin

import (
	"context"
	"net/http"
	"net/url"
	"testing"

	"github.com/stretchr/testify/assert"
)

type handler func(t *testing.T, req *http.Request) *http.Request

var test1Handler handler = func(t *testing.T, req *http.Request) *http.Request {
	t.Logf("req1.0: %p, req.URL: %p", req, req.URL)
	return req
}

var test2Handler handler = func(t *testing.T, req *http.Request) *http.Request {
	t.Logf("req2.0: %p, req.URL: %p", req, req.URL)

	saveCtx := req.Context()
	defer func() {
		t.Logf("req2.3: %p, req.URL: %p", req, req.URL)
		req = req.WithContext(saveCtx)
		t.Logf("req2.4: %p, req.URL: %p", req, req.URL)
	}()

	defer func() {
		t.Logf("req2.1: %p, req.URL: %p", req, req.URL)
		req.URL = &url.URL{
			Scheme: "https",
		}
		t.Logf("req2.2: %p, req.URL: %p", req, req.URL)
	}()

	req = req.WithContext(context.WithValue(req.Context(), "key", "value"))

	return req
}

var test3Handler handler = func(t *testing.T, req *http.Request) *http.Request {
	t.Logf("req3.0: %p, req.URL: %p", req, req.URL)
	t.Logf("req3.1: %v", req.Context().Value("key"))
	return req
}

func TestIssue_Middleware(t *testing.T) {
	req, err := http.NewRequest("POST", "/upload", nil)
	assert.NoError(t, err)
	req.URL = &url.URL{
		Scheme: "http",
	}

	t.Logf("req0.0: %p, req.URL: %p", req, req.URL)

	handlers := []handler{
		test1Handler,
		test2Handler,
		test3Handler,
	}
	for _, h := range handlers {
		req = h(t, req)
	}
}

output:

=== RUN   TestIssue_Middleware
    issue_test.go:55: req0.0: 0x140001b6500, req.URL: 0x14000130360
    issue_test.go:15: req1.0: 0x140001b6500, req.URL: 0x14000130360
    issue_test.go:20: req2.0: 0x140001b6500, req.URL: 0x14000130360
    issue_test.go:30: req2.1: 0x140001b6640, req.URL: 0x14000130360
    issue_test.go:34: req2.2: 0x140001b6640, req.URL: 0x140001303f0
    issue_test.go:24: req2.3: 0x140001b6640, req.URL: 0x140001303f0
    issue_test.go:26: req2.4: 0x140001b6780, req.URL: 0x140001303f0
    issue_test.go:43: req3.0: 0x140001b6640, req.URL: 0x140001303f0
    issue_test.go:44: req3.1: value
--- PASS: TestIssue_Middleware (0.00s)
PASS

instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go

instrumentation/github.com/gin-gonic/gin/otelgin/test/gintrace_test.go

dmathieu · 2025-01-14T08:21:31Z

instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go

@@ -85,6 +85,17 @@ func Middleware(service string, opts ...Option) gin.HandlerFunc {

 		// pass the span through the request context
 		c.Request = c.Request.WithContext(ctx)


Wouldn't we be fixing this rather more easily with the following:

Suggested change

c.Request = c.Request.WithContext(ctx)

c.Request = c.Request.Clone(ctx)

WithContext performs a shallow clone, and therefore doesn't clone the multipart form.
However, Clone performs a deep copy, and makes an explicit clone of MultipartForm.

The problem here is, go http request will call MultipartForm.RemoveAll() automatically when request finishes. However, the otelgin middleware creates a new request, and temporary files will be stored in this new request's MultipartForm field. Unfortunately, no one is calling MultipartForm.RemoveAll() on this new request.

A deep clone wouldn’t resolve the problem because the MultipartForm is also copied. A shallow clone might seem like it should work since the MultipartForm field is shared, but it doesn’t. This is because the MultipartForm field is lazily initialized—it’s only assigned a value when you parse the form. As a result, in the middleware, the field is nil.

Related issues: golang/go#58809, labstack/echo#2413

I think other OpenTelemetry middlewares, such as otelmux and otelehco, might also have the same issue.

What if we parse the form before making the new request?
Wouldn't we share the object then?

I think this approach could work, but it feels a bit unusual to enforce form parsing in the middleware.

Having to force remove multipart files is a bit unusual too.

@dmathieu maybe we should discuss this in the SIG since it might have implications in the other instrumentation modules

I won't be able to attend this week's meeting (it's the week where it's late for EU). But yes, feel free to bring it there.

@Yiling-J what are you thoughts on parsing the form in the middleware, the more I think about it, the more that seems to be the better approach, for example what about certain edge cases where down the chain middlewares might want to access the files

@akats7 Another option is to save c.Request to a variable before calling c.Request.WithContext(ctx). Then, in the defer function, if c.Request.MultipartForm != nil, we can reattach the MultipartForm to the original request. This way, there won’t be any automatic removing or parsing. If other middlewares handle the form correctly, this approach should work.
In http/server.go, the ServeHTTP method is called first, which invokes the Gin handlers/middlewares, and then finishRequest is called, which removes the temporary files.

I haven’t tested yet, but I think it should work. However, I’m considering opening an issue in Go to get their suggestions. Since it’s closely related to the http package and the WithContext method is widely used by middlewares and frameworks, I think they might be able to suggest best practice for this situation.

@akats7 It seems I can’t get any help from the Go team. I’m not in favor of parsing the form in the middleware, as this step should be explicitly done by the user. Parsing large forms can consume resources, and the ParseMultipartForm function has a maxMemory parameter that should also be decided by the user.

After giving this more thought over the past few days, I believe this is more of a Go design issue. The WithContext and ParseMultipartForm behavior is implicit, hard to notice, poorly documented, and lacks a clear best practice solution. I think the reattach approach might be better because it’s less aggressive and can likely be safely applied to other OpenTelemetry middlewares facing the same issue. The only downside, compared to RemoveAll, is that it relies on other middlewares to also handle WithContext and ParseMultipartForm correctly. If all middlewares do the right thing, net/http should eventually clean up the temporary files automatically.

Yiling-J · 2025-01-14T15:29:23Z

I skipped the failed test on the Windows platform. This is consistent with how Go handles form cleanup tests (https://github.com/golang/go/blob/6da16013ba4444e0d71540f68279f0283a92d05d/src/net/http/serve_test.go#L7028), so I think it’s fine.

fix(otelgin): remove multipartform temporary file

6875165

Yiling-J requested a review from a team as a code owner January 13, 2025 13:45

github-actions bot requested a review from akats7 January 13, 2025 13:45

Yiling-J added 3 commits January 13, 2025 22:44

fix lint

61b90e2

update changelog

a87ae01

fix lint

3131248

flc1125 reviewed Jan 13, 2025

View reviewed changes

Yiling-J added 2 commits January 13, 2025 23:23

update test

39c3fc5

remove unused var

bfa19b5

dmathieu reviewed Jan 14, 2025

View reviewed changes

addressing comments and fix test

65ce788

Yiling-J mentioned this pull request Jan 22, 2025

net/http: recommended way of handling MultipartForm and WithContext in middleware golang/go#71382

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(otelgin): remove multipartform temporary file #6609

fix(otelgin): remove multipartform temporary file #6609

Yiling-J commented Jan 13, 2025

linux-foundation-easycla bot commented Jan 13, 2025 •

edited

Loading

codecov bot commented Jan 13, 2025 •

edited

Loading

flc1125 Jan 13, 2025

Yiling-J Jan 13, 2025

flc1125 Jan 13, 2025

Yiling-J Jan 13, 2025

flc1125 Jan 13, 2025

flc1125 commented Jan 13, 2025 •

edited

Loading

dmathieu Jan 14, 2025

Yiling-J Jan 14, 2025

dmathieu Jan 14, 2025

Yiling-J Jan 14, 2025

dmathieu Jan 14, 2025

akats7 Jan 14, 2025

dmathieu Jan 14, 2025

akats7 Jan 19, 2025

Yiling-J Jan 20, 2025

Yiling-J Jan 27, 2025

Yiling-J commented Jan 14, 2025

		@@ -85,6 +85,17 @@ func Middleware(service string, opts ...Option) gin.HandlerFunc {

		// pass the span through the request context
		c.Request = c.Request.WithContext(ctx)

	c.Request = c.Request.WithContext(ctx)
	c.Request = c.Request.Clone(ctx)

fix(otelgin): remove multipartform temporary file #6609

Are you sure you want to change the base?

fix(otelgin): remove multipartform temporary file #6609

Conversation

Yiling-J commented Jan 13, 2025

linux-foundation-easycla bot commented Jan 13, 2025 • edited Loading

codecov bot commented Jan 13, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flc1125 commented Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yiling-J commented Jan 14, 2025

linux-foundation-easycla bot commented Jan 13, 2025 •

edited

Loading

codecov bot commented Jan 13, 2025 •

edited

Loading

flc1125 commented Jan 13, 2025 •

edited

Loading