Skip to content
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

context Value method issue tracking #4084

Open
FarmerChillax opened this issue Oct 30, 2024 · 2 comments
Open

context Value method issue tracking #4084

FarmerChillax opened this issue Oct 30, 2024 · 2 comments

Comments

@FarmerChillax
Copy link
Contributor

FarmerChillax commented Oct 30, 2024

problem locating the hasRequestContext function return False

gin/context.go

Line 1240 in 9c081de

func (c *Context) hasRequestContext() bool {

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 #3993 (comment)

@FarmerChillax FarmerChillax changed the title content Value method issue tracking context Value method issue tracking Oct 30, 2024
@HubertJan
Copy link

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
Copy link

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants