gin-gonic / gin

Gin is a HTTP web framework written in Go (Golang). It features a Martini-like API with much better performance -- up to 40 times faster. If you need smashing performance, get yourself some Gin.
https://gin-gonic.com/
MIT License
78.99k stars 8.03k forks source link

context `Value` method issue tracking #4084

Open FarmerChillax opened 2 weeks ago

FarmerChillax commented 2 weeks ago

problem locating the hasRequestContext function return Falsehttps://github.com/gin-gonic/gin/blob/9c081de9cdd1948f521d47d170d18cbc2981c33a/context.go#L1240

It seems that the problem is caused by the ContextWithFallback field, but I am not sure what this field does.

Solution:

// ...
app := gin.Default()
app.ContextWithFallback = true
// ...

Originally posted by @FarmerChillax in https://github.com/gin-gonic/gin/issues/3993#issuecomment-2172593121

HubertJan commented 4 days ago

I've done some research and identified the cause of the "strange" behaviour we encountered.

Flag Behaviour:

When ContextWithFallback is false, hasRequestContext always returns false, regardless of the actual request context. When ContextWithFallback is true, it checks if the current request has a valid context and only returns true if it does.

Summary:

ContextWithFallback is a backward compatibility flag introduced to avoid breaking changes with versions prior to v1.8.0. The previous behavior was unintuitive, but fixing it directly would have caused compatibility issues. Therefore, the ContextWithFallback flag was added, set to false by default to maintain legacy behaviour.

Edge Case in v1.8.0 Leading to this Flag:

In v1.8.0, a specific issue arose when a server reused a request context in an asynchronous Go routine. This context might already be closed when reused, leading to a canceled context error. Below the "original edge case":

func TestGinContextCancel(t *testing.T) {
    serv := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {
        return
    }))
    defer serv.Close()

    wg := &sync.WaitGroup{}

    r := gin.New()
    r.GET("/", func(ginctx *gin.Context) {
        wg.Add(1)
        ginctx = ginctx.Copy()

        // Start async goroutine for calling serv
        go func() {
            defer wg.Done()

            // Reusing the response context `ginctx` for a new request
            req, err := http.NewRequestWithContext(ginctx, http.MethodGet, serv.URL, nil)
            if err != nil {
                panic(err)
            }

            // With v1.8.0, `ginctx` is closed at this point, causing an error
            res, err := http.DefaultClient.Do(req)
            if err != nil {
                t.Error("Context is always canceled with Gin v1.8.0, a significant change from v1.7", err)
                return
            }

            if res.StatusCode != http.StatusOK {
                log.Println("Unexpected status code", res.Status)
                return
            }
        }()
    })

    go func() {
        if err := r.Run(":8080"); err != nil {
            panic(err)
        }
    }()

    // Trigger the route
    res, err := http.Get("http://127.1:8080/")
    if err != nil {
        panic(err)
    }

    if res.StatusCode != http.StatusOK {
        panic("Server returned an unexpected status")
    }

    wg.Wait()
}

Some links to the related issues: Original Edge case, #3166 Introduction of flag, #3172

HubertJan commented 4 days ago

To actually improve anything about the current situation, we would have to introduce breaking changes, which probably is not wanted. Therefore, I guess, all we can do is improve the docs?