C. Where is the possible cause of the difference between the pointer address of the request object and the pointer address of the r object? #2413
Comments
|
After repetition, the middleware c.request object is different from the r object, package main
import (
"errors"
"fmt"
"io"
"net/http"
"os"
"strings"
"github.com/labstack/echo-contrib/jaegertracing"
"github.com/labstack/echo/v4"
"github.com/opentracing/opentracing-go"
"github.com/opentracing/opentracing-go/log"
)
func upload(c echo.Context) error {
// Read form fields
name := c.FormValue("name")
email := c.FormValue("email")
//-----------
// Read file
//-----------
// Source
file, err := c.FormFile("file")
if err != nil {
return err
}
src, err := file.Open()
if err != nil {
return err
}
defer src.Close()
// Destination
dst, err := os.Create(file.Filename)
if err != nil {
return err
}
defer dst.Close()
// Copy
if _, err = io.Copy(dst, src); err != nil {
return err
}
return c.HTML(http.StatusOK, fmt.Sprintf("<p>File %s uploaded successfully with fields name=%s and email=%s.</p>", file.Filename, name, email))
}
func main() {
e := echo.New()
tracer := createMockTracer()
e.Use(jaegertracing.TraceWithConfig(jaegertracing.TraceConfig{
Tracer: tracer,
ComponentName: "EchoTracer",
OperationNameFunc: func(c echo.Context) string {
// This is an example of operation name customization
// In most cases default formatting is more than enough
req := c.Request()
opName := "HTTP " + req.Method
path := c.Path()
paramNames := c.ParamNames()
for _, name := range paramNames {
from := ":" + name
to := "{" + name + "}"
path = strings.ReplaceAll(path, from, to)
}
return opName + " " + path
},
}))
e.Static("/", "public")
e.POST("/upload", upload)
e.Logger.Fatal(e.Start(":1323"))
}
// Mock opentracing.Span
type mockSpan struct {
tracer opentracing.Tracer
tags map[string]interface{}
logs map[string]interface{}
opName string
finished bool
}
func createSpan(tracer opentracing.Tracer) *mockSpan {
return &mockSpan{
tracer: tracer,
tags: make(map[string]interface{}),
logs: make(map[string]interface{}),
}
}
func (sp *mockSpan) isFinished() bool {
return sp.finished
}
func (sp *mockSpan) getOpName() string {
return sp.opName
}
func (sp *mockSpan) getTag(key string) interface{} {
return sp.tags[key]
}
func (sp *mockSpan) getLog(key string) interface{} {
return sp.logs[key]
}
func (sp *mockSpan) Finish() {
sp.finished = true
}
func (sp *mockSpan) FinishWithOptions(opts opentracing.FinishOptions) {
}
func (sp *mockSpan) Context() opentracing.SpanContext {
return nil
}
func (sp *mockSpan) SetOperationName(operationName string) opentracing.Span {
sp.opName = operationName
return sp
}
func (sp *mockSpan) SetTag(key string, value interface{}) opentracing.Span {
sp.tags[key] = value
return sp
}
func (sp *mockSpan) LogFields(fields ...log.Field) {
}
func (sp *mockSpan) LogKV(alternatingKeyValues ...interface{}) {
for i := 0; i < len(alternatingKeyValues); i += 2 {
ikey := alternatingKeyValues[i]
value := alternatingKeyValues[i+1]
if key, ok := ikey.(string); ok {
sp.logs[key] = value
}
}
}
func (sp *mockSpan) SetBaggageItem(restrictedKey, value string) opentracing.Span {
return sp
}
func (sp *mockSpan) BaggageItem(restrictedKey string) string {
return ""
}
func (sp *mockSpan) Tracer() opentracing.Tracer {
return sp.tracer
}
func (sp *mockSpan) LogEvent(event string) {
}
func (sp *mockSpan) LogEventWithPayload(event string, payload interface{}) {
}
func (sp *mockSpan) Log(data opentracing.LogData) {
}
// Mock opentracing.Tracer
type mockTracer struct {
span *mockSpan
hasStartSpanWithOption bool
}
func (tr *mockTracer) currentSpan() *mockSpan {
return tr.span
}
func (tr *mockTracer) StartSpan(operationName string, opts ...opentracing.StartSpanOption) opentracing.Span {
tr.hasStartSpanWithOption = len(opts) > 0
if tr.span != nil {
tr.span.opName = operationName
return tr.span
}
span := createSpan(tr)
span.opName = operationName
return span
}
func (tr *mockTracer) Inject(sm opentracing.SpanContext, format interface{}, carrier interface{}) error {
return nil
}
func (tr *mockTracer) Extract(format interface{}, carrier interface{}) (opentracing.SpanContext, error) {
if tr.span != nil {
return nil, nil
}
return nil, errors.New("no span")
}
func createMockTracer() *mockTracer {
tracer := mockTracer{}
span := createSpan(&tracer)
tracer.span = span
return &tracer
} |
CLICK ME
|
|
You are using jaegertracing middleware and it replaces request here https://github.com/labstack/echo-contrib/blob/5bb0f68fcbfa500642b965c9ca0be75230e19417/jaegertracing/jaegertracing.go#L200 |
|
Yes, how can I keep the pointer address unchanged? Because I found that if the r object has a file, the http response will eventually clean up the file, and it can't go after a lot of logic changes |
|
How do you test this? When I post file as multipart NB: You can not send/post files with |
|
Use the code I gave above to reproduce the problem. Add a breakpoint on line 667 of the 'ServeHTTP' method of echo to reproduce the problem. The version used is v4.10.2. |
|
test curl |
CLICK ME |
x@x:~/$ curl -v -F "name=test" -F "email=xxx" -F file=@Makefile http://localhost:8080/upload
* Trying 127.0.0.1:8080...
* Connected to localhost (127.0.0.1) port 8080 (#0)
> POST /upload HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.85.0
> Accept: */*
> Content-Length: 3022
> Content-Type: multipart/form-data; boundary=------------------------826930f239e33ed9
>
* We are completely uploaded and fine
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Content-Type: text/html; charset=UTF-8
< Date: Wed, 01 Mar 2023 13:11:46 GMT
< Content-Length: 79
<
* Connection #0 to host localhost left intact
<p>File Makefile uploaded successfully with fields name=test and email=xxx.</p>I am using your example with different port |
|
Use the following code to test package main
import (
["errors"](https://pkg.go.dev/errors)
["fmt"](https://pkg.go.dev/fmt)
["io"](https://pkg.go.dev/io)
["net/http"](https://pkg.go.dev/net/http)
["os"](https://pkg.go.dev/os)
["strings"](https://pkg.go.dev/strings)
https://github.com/labstack/echo-contrib/tree/master/jaegertracing
"github.com/labstack/echo/v4"
"github.com/opentracing/opentracing-go"
https://github.com/opentracing/opentracing-go/tree/master/log
)
func upload(c echo.Context) error {
// Read form fields
name := c.FormValue("name")
email := c.FormValue("email")
//-----------
// Read file
//-----------
// Source
file, err := c.FormFile("file")
if err != nil {
return err
}
src, err := file.Open()
if err != nil {
return err
}
defer src.Close()
// Destination
dst, err := os.Create(file.Filename)
if err != nil {
return err
}
defer dst.Close()
// Copy
if _, err = io.Copy(dst, src); err != nil {
return err
}
return c.HTML(http.StatusOK, fmt.Sprintf("<p>File %s uploaded successfully with fields name=%s and email=%s.</p>", file.Filename, name, email))
}
func main() {
e := echo.New()
tracer := createMockTracer()
e.Use(jaegertracing.TraceWithConfig(jaegertracing.TraceConfig{
Tracer: tracer,
ComponentName: "EchoTracer",
OperationNameFunc: func(c echo.Context) string {
// This is an example of operation name customization
// In most cases default formatting is more than enough
req := c.Request()
opName := "HTTP " + req.Method
path := c.Path()
paramNames := c.ParamNames()
for _, name := range paramNames {
from := ":" + name
to := "{" + name + "}"
path = strings.ReplaceAll(path, from, to)
}
return opName + " " + path
},
}))
e.Static("/", "public")
e.POST("/upload", upload)
e.Logger.Fatal(e.Start(":1323"))
}
// Mock opentracing.Span
type mockSpan struct {
tracer opentracing.Tracer
tags map[string]interface{}
logs map[string]interface{}
opName string
finished bool
}
func createSpan(tracer opentracing.Tracer) *mockSpan {
return &mockSpan{
tracer: tracer,
tags: make(map[string]interface{}),
logs: make(map[string]interface{}),
}
}
func (sp *mockSpan) isFinished() bool {
return sp.finished
}
func (sp *mockSpan) getOpName() string {
return sp.opName
}
func (sp *mockSpan) getTag(key string) interface{} {
return sp.tags[key]
}
func (sp *mockSpan) getLog(key string) interface{} {
return sp.logs[key]
}
func (sp *mockSpan) Finish() {
sp.finished = true
}
func (sp *mockSpan) FinishWithOptions(opts opentracing.FinishOptions) {
}
func (sp *mockSpan) Context() opentracing.SpanContext {
return nil
}
func (sp *mockSpan) SetOperationName(operationName string) opentracing.Span {
sp.opName = operationName
return sp
}
func (sp *mockSpan) SetTag(key string, value interface{}) opentracing.Span {
sp.tags[key] = value
return sp
}
func (sp *mockSpan) LogFields(fields ...log.Field) {
}
func (sp *mockSpan) LogKV(alternatingKeyValues ...interface{}) {
for i := 0; i < len(alternatingKeyValues); i += 2 {
ikey := alternatingKeyValues[i]
value := alternatingKeyValues[i+1]
if key, ok := ikey.(string); ok {
sp.logs[key] = value
}
}
}
func (sp *mockSpan) SetBaggageItem(restrictedKey, value string) opentracing.Span {
return sp
}
func (sp *mockSpan) BaggageItem(restrictedKey string) string {
return ""
}
func (sp *mockSpan) Tracer() opentracing.Tracer {
return sp.tracer
}
func (sp *mockSpan) LogEvent(event string) {
}
func (sp *mockSpan) LogEventWithPayload(event string, payload interface{}) {
}
func (sp *mockSpan) Log(data opentracing.LogData) {
}
// Mock opentracing.Tracer
type mockTracer struct {
span *mockSpan
hasStartSpanWithOption bool
}
func (tr *mockTracer) currentSpan() *mockSpan {
return tr.span
}
func (tr *mockTracer) StartSpan(operationName string, opts ...opentracing.StartSpanOption) opentracing.Span {
tr.hasStartSpanWithOption = len(opts) > 0
if tr.span != nil {
tr.span.opName = operationName
return tr.span
}
span := createSpan(tr)
span.opName = operationName
return span
}
func (tr *mockTracer) Inject(sm opentracing.SpanContext, format interface{}, carrier interface{}) error {
return nil
}
func (tr *mockTracer) Extract(format interface{}, carrier interface{}) (opentracing.SpanContext, error) {
if tr.span != nil {
return nil, nil
}
return nil, errors.New("no span")
}
func createMockTracer() *mockTracer {
tracer := mockTracer{}
span := createSpan(&tracer)
tracer.span = span
return &tracer
} |
|
This is exactly the same (except port nr) as I use. And I set breakpoint in |
CLICK ME You can see that the 'WithContext' method of the http request object will reinitialize a request object to overwrite the current request object, so this is the problem that causes the pointer address to change. How can I ensure that it remains unchanged? |
|
Yes, there is no problem with the way you operate. The problem I said is that after calling the 'WithContext' method of http request through some middleware, the pointer address of the original request variable changes, which will lead to the final inconsistency between the r object and the 'c.request' object of the 'ServeHTTP' method of echo, thus unable to release file resources |
|
You can not with current implementation of Please describe your problem better and please do not at the moment fixate on the symptom You said that
What do you mean by |
CLICK ME |
|
So you are describing leaking file descriptors or leaking memory? and please do not post these pictures - I already saw from first picture that pointer address is different. Lets discuss what is the problem. |
|
It means that c.request.MultipartForm has a value, while r.MultipartForm has no value, |
|
https://cs.opensource.google/go/go/+/refs/tags/go1.20.1:src/net/http/server.go;l=1660 It means that c.request.MultipartForm has a value, while r.MultipartForm has no value, so http's server.go if w.req.MultipartForm != nil {
w.req.MultipartForm.RemoveAll()
}Unable to execute The temporary file was not deleted |
|
alright, to summarize this situation: we are leaking temporary files because when req = req.WithContext(opentracing.ContextWithSpan(req.Context(), sp))
c.SetRequest(req)problem with that is when now these temporary form files are cleaned up at the end of the request by go standard library HERE but http server does not have access to that new Request object - it still works on that old Request object that does not have parsed form, therefore does not know anything about temporary files. |
Yes, how can I solve this problem? Do you manually call 'MultipartForm. RemoveAll()' in the ServHTTP method of echo if c.request.MultipartForm != nil {
c.request.MultipartForm.RemoveAll()
} |
|
you can clean multipartform at the end of handler by adding defer file, err := c.FormFile("file")
if err != nil {
return err
}
defer func() {
c.Request().MultipartForm.RemoveAll()
}() |
Yes, I know I can call it manually, but should the framework theoretically support this function? Because the finishRequest () of the default http standard library does this, the general user will not think of using the WithContext method to cause finishRequest to fail to work normally, but will have to manually call c.Request(). MultipartForm. RemoveAll() to clean up the temporary file manually |
|
Yep, req = req.WithContext(opentracing.ContextWithSpan(req.Context(), sp))
c.SetRequest(req)
defer func() {
if req.MultipartForm != nil {
req.MultipartForm.RemoveAll()
}
}()to clean up the problem it causes. p.s. repo for that mw is https://github.com/labstack/echo-contrib not this repository |
Echo does not guarantee that the middleware does not call the WithContext method. I think it should be cleaned manually in the ServHTTP method of echo |
|
|
|
I'll create PR for |
|
But the user does not know that the middleware will not clean up the file resources after calling WithContext |
|
We can't guarantee that the middleware used by the user will not use the finishRequest() method of the http standard library after calling WithContext |
|
We can not guarantee that there would not be multiple chained middlewares replacing request objects in row. Therefore we can not assume that at the end of problem must be fixed where it is introduced. - |
|
I'll merge that fix in echo-contrib in couple of hours and tag a new release. |
|
Thank you |
|
@JellyZero try this https://github.com/labstack/echo-contrib/releases/tag/v0.14.1 and if it is not enough - create issue in that repository |
Issue Description
Checklist
Expected behaviour
Actual behaviour
Steps to reproduce
Working code to debug
Version/commit
C. Where is the possible cause of the difference between the pointer address of the request object and the pointer address of the r object? After the middleware processing, how can the middleware overwrite the pointer object of the original r?
The text was updated successfully, but these errors were encountered: