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

feat(context): add ContextWithFallback feature flag (#3166) #3172

Merged
merged 1 commit into from Jun 6, 2022

Conversation

wei840222
Copy link
Contributor

@wei840222 wei840222 commented Jun 2, 2022

due to breaking change in #3166

@codecov
Copy link

codecov bot commented Jun 2, 2022

Codecov Report

Merging #3172 (9a0d2d2) into master (58303bd) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3172   +/-   ##
=======================================
  Coverage   98.22%   98.22%           
=======================================
  Files          43       43           
  Lines        3148     3148           
=======================================
  Hits         3092     3092           
  Misses         44       44           
  Partials       12       12           
Flag Coverage Δ
go-1.14 98.22% <100.00%> (ø)
go-1.15 98.06% <100.00%> (+0.02%) ⬆️
go-1.16 98.03% <100.00%> (ø)
go-1.17 97.96% <100.00%> (ø)
go-1.18 97.96% <100.00%> (ø)
macos-latest 98.22% <100.00%> (+0.18%) ⬆️
nomsgpack 98.19% <100.00%> (ø)
ubuntu-latest 98.22% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
gin.go 99.18% <ø> (ø)
context.go 96.76% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58303bd...9a0d2d2. Read the comment docs.

@appleboy
Copy link
Member

appleboy commented Jun 2, 2022

@wei840222 From #3166 (comment) 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. so we don't need to re-open all related issues.

@wei840222
Copy link
Contributor Author

wei840222 commented Jun 2, 2022

oh I see, let me do it

@wei840222
Copy link
Contributor Author

wei840222 commented Jun 2, 2022

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

appleboy commented Jun 2, 2022

Testing code

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()

			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
			}
		}()
	})
	go func() {
		err := r.Run(":8080")
		if 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)
	}

	wg.Wait()
}

#3166 (comment)

@appleboy
Copy link
Member

appleboy commented Jun 2, 2022

#2751
#2769

We need to review the above two PR and make it backward compatible.

@jerome-laforge
Copy link
Contributor

jerome-laforge commented Jun 2, 2022

@appleboy & @wei840222

maybe to avoid flawky test by ensuring 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 closing the channel ensureRequestIsOver

you can use this:

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)
	}

	close(ensureRequestIsOver)

	if res.StatusCode != http.StatusOK {
		panic(err)
	}

	wg.Wait()
}

@wei840222 wei840222 changed the title Fix: Revert "fallback Context.Deadline() Context.Done() Context.Err() to Context.Request.Context() (#2769)" feat(context): add ContextWithFallback feature flag for enable fallback Context.Deadline(), Context.Done(), Context.Err() and Context.Value() (#3166) Jun 6, 2022
…ck Context.Deadline(), Context.Done(), Context.Err() and Context.Value()
@wei840222 wei840222 changed the title feat(context): add ContextWithFallback feature flag for enable fallback Context.Deadline(), Context.Done(), Context.Err() and Context.Value() (#3166) feat(context): add ContextWithFallback feature flag for enable fallback Context.Deadline(), Context.Done(), Context.Err() and Context.Value() Jun 6, 2022
@wei840222 wei840222 changed the title feat(context): add ContextWithFallback feature flag for enable fallback Context.Deadline(), Context.Done(), Context.Err() and Context.Value() feat(context): add ContextWithFallback feature flag for enable fallback Context.Deadline(), Context.Done(), Context.Err() and Context.Value() (#3166) Jun 6, 2022
@wei840222
Copy link
Contributor Author

wei840222 commented Jun 6, 2022

Hi @appleboy and @jerome-laforge
here is the version with feature for enable context with fallback (default is disable)

@jerome-laforge
Copy link
Contributor

jerome-laforge commented Jun 6, 2022

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.

@appleboy appleboy changed the title feat(context): add ContextWithFallback feature flag for enable fallback Context.Deadline(), Context.Done(), Context.Err() and Context.Value() (#3166) feat(context): add ContextWithFallback feature flag (#3166) Jun 6, 2022
@appleboy appleboy merged commit f197a8b into gin-gonic:master Jun 6, 2022
23 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants