Repository: gin-gonic/gin
Pull request #3172 (2022-06-02 03:50:58)

feat(context): add ContextWithFallback feature flag (#3166)
due to breaking change in https://github.com/gin-gonic/gin/issues/3166


codecov[bot]: # [Codecov](https://codecov.io/gh/gin-gonic/gin/pull/3172?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic) Report
> Merging [#3172](https://codecov.io/gh/gin-gonic/gin/pull/3172?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic) (045553e) into [master](https://codecov.io/gh/gin-gonic/gin/commit/58303bde7da71ba6252190433b112a138e201477?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic) (58303bd) will **decrease** coverage by `0.29%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##           master    #3172      +/-   ##
==========================================
- Coverage   98.22%   97.93%   -0.30%     
==========================================
  Files          43       40       -3     
  Lines        3148     3093      -55     
==========================================
- Hits         3092     3029      -63     
- Misses         44       50       +6     
- Partials       12       14       +2     
```

| Flag | Coverage Δ | |
|---|---|---|
| go-1.14 | `?` | |
| go-1.15 | `?` | |
| go-1.16 | `∅ <ø> (∅)` | |
| go-1.17 | `97.93% <100.00%> (-0.04%)` | :arrow_down: |
| go-1.18 | `97.93% <100.00%> (-0.04%)` | :arrow_down: |
| macos-latest | `∅ <ø> (∅)` | |
| nomsgpack | `97.93% <100.00%> (-0.27%)` | :arrow_down: |
| ubuntu-latest | `97.93% <100.00%> (-0.30%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/gin-gonic/gin/pull/3172?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic) | Coverage Δ | |
|---|---|---|
| [context.go](https://codecov.io/gh/gin-gonic/gin/pull/3172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic#diff-Y29udGV4dC5nbw==) | `96.19% <100.00%> (-0.57%)` | :arrow_down: |
| [debug.go](https://codecov.io/gh/gin-gonic/gin/pull/3172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic#diff-ZGVidWcuZ28=) | `91.66% <0.00%> (-8.34%)` | :arrow_down: |
| [binding/msgpack.go](https://codecov.io/gh/gin-gonic/gin/pull/3172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic#diff-YmluZGluZy9tc2dwYWNrLmdv) | | |
| [binding/binding.go](https://codecov.io/gh/gin-gonic/gin/pull/3172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic#diff-YmluZGluZy9iaW5kaW5nLmdv) | | |
| [render/msgpack.go](https://codecov.io/gh/gin-gonic/gin/pull/3172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic#diff-cmVuZGVyL21zZ3BhY2suZ28=) | | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/gin-gonic/gin/pull/3172?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/gin-gonic/gin/pull/3172?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic). Last update [58303bd...045553e](https://codecov.io/gh/gin-gonic/gin/pull/3172?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic).

appleboy: @wei840222 From  https://github.com/gin-gonic/gin/issues/3166#issuecomment-1143570149 comment

> Add a new flag to make it backward compatible.

We can add a new flag and let the developer decide when to turn on the feature.
wei840222: oh I see, let me do it
wei840222: @appleboy 
the one more thing I wanna check
The scope of the flag should include Context.Deadline(), Context.Done(), Context.Err(), Context.Value() or only Context.Deadline(), Context.Done(), Context.Err() ?
appleboy: https://github.com/gin-gonic/gin/pull/2751
https://github.com/gin-gonic/gin/pull/2769

We need to review the above two PR and make it backward compatible.


jerome-laforge: @appleboy & @wei840222 

maybe to avoid flawky tests: 
- ensure that :8080 is always opened (as initially done into goroutine)
- request on  http://127.0.0.1:8080/ is done before launch async task with channel

you can use this:

```go
func TestGinContextCancel(t *testing.T) {
	serv := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {
		return
	}))
	defer serv.Close()

	ensureRequestIsOver := make(chan struct{})

	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()

			<-ensureRequestIsOver // ensure request on http://127.0.0.1:8080/ is done

			req, err := http.NewRequestWithContext(ginctx, http.MethodGet, serv.URL, nil)
			if err != nil {
				panic(err)
			}

			res, err := http.DefaultClient.Do(req)
			if err != nil {
				// context is always canceled with gin v1.8.0, it is big breaking change with gin v1.7
				t.Error("context is always canceled with gin v1.8.0, it is big breaking change with gin v1.7", err)
				return
			}

			if res.StatusCode != http.StatusOK {
				log.Println("unexpected status code ", res.Status)
				return
			}
		}()
	})

	l, err := net.Listen("tcp", ":8080")
	if err != nil {
		panic(err)
	}
	go func() {
		s := &http.Server{
			Handler: r,
		}

		if err := s.Serve(l); err != nil {
			panic(err)
		}
	}()

	res, err := http.Get("http://127.0.0.1:8080/")
	if err != nil {
		panic(err)
	}

	if res.StatusCode != http.StatusOK {
		panic(err)
	}

	close(ensureRequestIsOver)

	wg.Wait()
}
```
wei840222: Hi @appleboy  and @jerome-laforge 
here is the version with feature for enable context with fallback (default is disable)
jerome-laforge: > here is the version with feature for enable context with fallback (default is disable)

Hi @wei840222,
I have just tested with `replace github.com/gin-gonic/gin v1.8.0 => github.com/wei840222/gin v1.8.1-0.20220606034057-9a0d2d27c9a7` in my go.mod. 
It works fine on my end.

Patch:
diff --git a/context.go b/context.go
index 6b25b3a..b1ad95e 100644
--- a/context.go
+++ b/context.go
@@ -1158,7 +1158,7 @@ func (c *Context) SetAccepted(formats ...string) {
 
 // Deadline returns that there is no deadline (ok==false) when c.Request has no Context.
 func (c *Context) Deadline() (deadline time.Time, ok bool) {
-	if c.Request == nil || c.Request.Context() == nil {
+	if !c.engine.ContextWithFallback || c.Request == nil || c.Request.Context() == nil {
 		return
 	}
 	return c.Request.Context().Deadline()
@@ -1166,7 +1166,7 @@ func (c *Context) Deadline() (deadline time.Time, ok bool) {
 
 // Done returns nil (chan which will wait forever) when c.Request has no Context.
 func (c *Context) Done() <-chan struct{} {
-	if c.Request == nil || c.Request.Context() == nil {
+	if !c.engine.ContextWithFallback || c.Request == nil || c.Request.Context() == nil {
 		return nil
 	}
 	return c.Request.Context().Done()
@@ -1174,7 +1174,7 @@ func (c *Context) Done() <-chan struct{} {
 
 // Err returns nil when c.Request has no Context.
 func (c *Context) Err() error {
-	if c.Request == nil || c.Request.Context() == nil {
+	if !c.engine.ContextWithFallback || c.Request == nil || c.Request.Context() == nil {
 		return nil
 	}
 	return c.Request.Context().Err()
@@ -1195,7 +1195,7 @@ func (c *Context) Value(key any) any {
 			return val
 		}
 	}
-	if c.Request == nil || c.Request.Context() == nil {
+	if !c.engine.ContextWithFallback || c.Request == nil || c.Request.Context() == nil {
 		return nil
 	}
 	return c.Request.Context().Value(key)
diff --git a/gin.go b/gin.go
index e516360..f932429 100644
--- a/gin.go
+++ b/gin.go
@@ -147,6 +147,9 @@ type Engine struct {
 	// UseH2C enable h2c support.
 	UseH2C bool
 
+	// ContextWithFallback enable fallback Context.Deadline(), Context.Done(), Context.Err() and Context.Value() when Context.Request.Context() is not nil.
+	ContextWithFallback bool
+
 	delims           render.Delims
 	secureJSONPrefix string
 	HTMLRender       render.HTMLRender


Test patch:
diff --git a/context_test.go b/context_test.go
index f9f0c1d..6c0be54 100644
--- a/context_test.go
+++ b/context_test.go
@@ -2097,12 +2097,18 @@ func TestRemoteIPFail(t *testing.T) {
 }
 
 func TestContextWithFallbackDeadlineFromRequestContext(t *testing.T) {
-	c := &Context{}
+	c, _ := CreateTestContext(httptest.NewRecorder())
+	// enable ContextWithFallback feature flag
+	c.engine.ContextWithFallback = true
+
 	deadline, ok := c.Deadline()
 	assert.Zero(t, deadline)
 	assert.False(t, ok)
 
-	c2 := &Context{}
+	c2, _ := CreateTestContext(httptest.NewRecorder())
+	// enable ContextWithFallback feature flag
+	c2.engine.ContextWithFallback = true
+
 	c2.Request, _ = http.NewRequest(http.MethodGet, "/", nil)
 	d := time.Now().Add(time.Second)
 	ctx, cancel := context.WithDeadline(context.Background(), d)
@@ -2114,10 +2120,16 @@ func TestContextWithFallbackDeadlineFromRequestContext(t *testing.T) {
 }
 
 func TestContextWithFallbackDoneFromRequestContext(t *testing.T) {
-	c := &Context{}
+	c, _ := CreateTestContext(httptest.NewRecorder())
+	// enable ContextWithFallback feature flag
+	c.engine.ContextWithFallback = true
+
 	assert.Nil(t, c.Done())
 
-	c2 := &Context{}
+	c2, _ := CreateTestContext(httptest.NewRecorder())
+	// enable ContextWithFallback feature flag
+	c2.engine.ContextWithFallback = true
+
 	c2.Request, _ = http.NewRequest(http.MethodGet, "/", nil)
 	ctx, cancel := context.WithCancel(context.Background())
 	c2.Request = c2.Request.WithContext(ctx)
@@ -2126,10 +2138,16 @@ func TestContextWithFallbackDoneFromRequestContext(t *testing.T) {
 }
 
 func TestContextWithFallbackErrFromRequestContext(t *testing.T) {
-	c := &Context{}
+	c, _ := CreateTestContext(httptest.NewRecorder())
+	// enable ContextWithFallback feature flag
+	c.engine.ContextWithFallback = true
+
 	assert.Nil(t, c.Err())
 
-	c2 := &Context{}
+	c2, _ := CreateTestContext(httptest.NewRecorder())
+	// enable ContextWithFallback feature flag
+	c2.engine.ContextWithFallback = true
+
 	c2.Request, _ = http.NewRequest(http.MethodGet, "/", nil)
 	ctx, cancel := context.WithCancel(context.Background())
 	c2.Request = c2.Request.WithContext(ctx)
@@ -2138,9 +2156,9 @@ func TestContextWithFallbackErrFromRequestContext(t *testing.T) {
 	assert.EqualError(t, c2.Err(), context.Canceled.Error())
 }
 
-type contextKey string
-
 func TestContextWithFallbackValueFromRequestContext(t *testing.T) {
+	type contextKey string
+
 	tests := []struct {
 		name             string
 		getContextAndKey func() (*Context, any)
@@ -2150,7 +2168,9 @@ func TestContextWithFallbackValueFromRequestContext(t *testing.T) {
 			name: "c with struct context key",
 			getContextAndKey: func() (*Context, any) {
 				var key struct{}
-				c := &Context{}
+				c, _ := CreateTestContext(httptest.NewRecorder())
+				// enable ContextWithFallback feature flag
+				c.engine.ContextWithFallback = true
 				c.Request, _ = http.NewRequest("POST", "/", nil)
 				c.Request = c.Request.WithContext(context.WithValue(context.TODO(), key, "value"))
 				return c, key
@@ -2160,7 +2180,9 @@ func TestContextWithFallbackValueFromRequestContext(t *testing.T) {
 		{
 			name: "c with string context key",
 			getContextAndKey: func() (*Context, any) {
-				c := &Context{}
+				c, _ := CreateTestContext(httptest.NewRecorder())
+				// enable ContextWithFallback feature flag
+				c.engine.ContextWithFallback = true
 				c.Request, _ = http.NewRequest("POST", "/", nil)
 				c.Request = c.Request.WithContext(context.WithValue(context.TODO(), contextKey("key"), "value"))
 				return c, contextKey("key")
@@ -2170,7 +2192,10 @@ func TestContextWithFallbackValueFromRequestContext(t *testing.T) {
 		{
 			name: "c with nil http.Request",
 			getContextAndKey: func() (*Context, any) {
-				c := &Context{}
+				c, _ := CreateTestContext(httptest.NewRecorder())
+				// enable ContextWithFallback feature flag
+				c.engine.ContextWithFallback = true
+				c.Request = nil
 				return c, "key"
 			},
 			value: nil,
@@ -2178,7 +2203,9 @@ func TestContextWithFallbackValueFromRequestContext(t *testing.T) {
 		{
 			name: "c with nil http.Request.Context()",
 			getContextAndKey: func() (*Context, any) {
-				c := &Context{}
+				c, _ := CreateTestContext(httptest.NewRecorder())
+				// enable ContextWithFallback feature flag
+				c.engine.ContextWithFallback = true
 				c.Request, _ = http.NewRequest("POST", "/", nil)
 				return c, "key"
 			},
@@ -2193,6 +2220,70 @@ func TestContextWithFallbackValueFromRequestContext(t *testing.T) {
 	}
 }
 
+func TestContextCopyShouldNotCancel(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer srv.Close()
+
+	ensureRequestIsOver := make(chan struct{})
+
+	wg := &sync.WaitGroup{}
+
+	r := New()
+	r.GET("/", func(ginctx *Context) {
+		wg.Add(1)
+
+		ginctx = ginctx.Copy()
+
+		// start async goroutine for calling srv
+		go func() {
+			defer wg.Done()
+
+			<-ensureRequestIsOver // ensure request is done
+
+			req, err := http.NewRequestWithContext(ginctx, http.MethodGet, srv.URL, nil)
+			must(err)
+
+			res, err := http.DefaultClient.Do(req)
+			if err != nil {
+				t.Error(fmt.Errorf("request error: %w", err))
+				return
+			}
+
+			if res.StatusCode != http.StatusOK {
+				t.Error(fmt.Errorf("unexpected status code: %s", res.Status))
+			}
+		}()
+	})
+
+	l, err := net.Listen("tcp", ":0")
+	must(err)
+	go func() {
+		s := &http.Server{
+			Handler: r,
+		}
+
+		must(s.Serve(l))
+	}()
+
+	addr := strings.Split(l.Addr().String(), ":")
+	res, err := http.Get(fmt.Sprintf("http://127.0.0.1:%s/", addr[len(addr)-1]))
+	if err != nil {
+		t.Error(fmt.Errorf("request error: %w", err))
+		return
+	}
+
+	close(ensureRequestIsOver)
+
+	if res.StatusCode != http.StatusOK {
+		t.Error(fmt.Errorf("unexpected status code: %s", res.Status))
+		return
+	}
+
+	wg.Wait()
+}
+
 func TestContextAddParam(t *testing.T) {
 	c := &Context{}
 	id := "id"