-
Notifications
You must be signed in to change notification settings - Fork 585
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(otelgin): remove multipartform temporary file #6609
base: main
Are you sure you want to change the base?
Changes from all commits
6875165
61b90e2
a87ae01
3131248
39c3fc5
bfa19b5
65ce788
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,6 +85,20 @@ | |
|
||
// pass the span through the request context | ||
c.Request = c.Request.WithContext(ctx) | ||
defer func() { | ||
// as we have created new http.Request object we need to make sure that temporary files created to hold MultipartForm | ||
// files are cleaned up. This is done by http.Server at the end of request lifecycle but Server does not | ||
// have reference to our new Request instance therefore it is our responsibility to fix the mess we caused. | ||
// | ||
// 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering, if there's another middleware layer outside this one, would it be removed because of this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see: #6609 (comment) |
||
err := c.Request.MultipartForm.RemoveAll() | ||
if err != nil { | ||
otel.Handle(err) | ||
} | ||
} | ||
}() | ||
|
||
// serve the request to the next middleware | ||
c.Next() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't we be fixing this rather more easily with the following:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we parse the form before making the new request?
Wouldn't we share the object then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this approach could work, but it feels a bit unusual to enforce form parsing in the middleware.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having to force remove multipart files is a bit unusual too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm probably more in favor of doing the initialization on the original request as well, explicitly handling clean up logic does seem a bit strange
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmathieu maybe we should discuss this in the SIG since it might have implications in the other instrumentation modules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.