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

With gin v1.8.0, gin.Context is canceled within goroutine #3166

Closed
jerome-laforge opened this issue May 31, 2022 · 13 comments
Closed

With gin v1.8.0, gin.Context is canceled within goroutine #3166

jerome-laforge opened this issue May 31, 2022 · 13 comments

Comments

@jerome-laforge
Copy link
Contributor

jerome-laforge commented May 31, 2022

Description

With gin v1.8.0, the gin.Context is canceled when we use it within goroutine for doing async task (so this task can be executed after the reply to http client that doing request on gin router). This is big breaking change in regard of gin v1.7

How to reproduce

This test below is red with gin v1.8.0 but it was green with v1.7.7.

package main

import (
	"log"
	"net/http"
	"net/http/httptest"
	"sync"
	"testing"

	"github.com/gin-gonic/gin"
)

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.1:8080/")
	if err != nil {
		panic(err)
	}

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

	wg.Wait()
}

Expectations

With gin v1.8.0, the provided test must be green

Actual result

With gin v1.8.0, the provided test is red

Environment

  • go version: 1.18
  • gin version (or commit ref): 1.8.0
  • operating system: linux
@jerome-laforge jerome-laforge changed the title Context is canceled within goroutine With gin 1.8.0, gin.Context is canceled within goroutine May 31, 2022
@jerome-laforge jerome-laforge changed the title With gin 1.8.0, gin.Context is canceled within goroutine With gin v1.8.0, gin.Context is canceled within goroutine May 31, 2022
@jerome-laforge
Copy link
Contributor Author

jerome-laforge commented May 31, 2022

In my humble opinion, the root cause of this issue is #2769

@zetaab
Copy link

zetaab commented May 31, 2022

we are seeing issues with this as well. I have now reverted 1.8.0 from quite many services.

@wei840222
Copy link
Contributor

wei840222 commented Jun 1, 2022

I have a proposal
Maybe context.Copy()
The returned Context content is wrapped with an orphan context so that it will not be canceled by the source context
I don't know if everyone thinks this is appropriate

@jerome-laforge
Copy link
Contributor Author

jerome-laforge commented Jun 1, 2022

I don't know if everyone thinks this is appropriate

Maybe, but I think this kind of evolution can't be done in MINOR version change (1.7.x -> 1.8.x) as it is a big API breaking change.
The API breaking change must be done in MAJOR version change (1.x -> 2.x) according to semantic versioning

@appleboy
Copy link
Member

appleboy commented Jun 1, 2022

@thinkerou What do you think about this? The following is my proposal.

  1. Revert some breaking change commits and move to the v2 version.
  2. Add a new flag to make it backward compatible.

@thinkerou
Copy link
Member

thinkerou commented Jun 1, 2022

@appleboy approve 2, the pr solved Trusted Proxies issue.

@appleboy
Copy link
Member

appleboy commented Jun 2, 2022

@wei840222 Can you make a new PR to resolve the issue?

@appleboy appleboy added this to the v1.8.1 milestone Jun 2, 2022
@wei840222
Copy link
Contributor

wei840222 commented Jun 2, 2022

@appleboy Yes I can

wei840222 added a commit to wei840222/gin that referenced this issue Jun 2, 2022
… to Context.Request.Context() (gin-gonic#2769)"

due to breaking change in issue gin-gonic#3166
@wei840222
Copy link
Contributor

wei840222 commented Jun 2, 2022

Hi, @appleboy
here is the pr #3172

wei840222 added a commit to wei840222/gin that referenced this issue Jun 6, 2022
…ck Context.Deadline(), Context.Done(), Context.Err() and Context.Value() (gin-gonic#3166)
appleboy pushed a commit that referenced this issue Jun 6, 2022
Enable fallback Context.Deadline(), Context.Done(), Context.Err() and Context.Value()
@appleboy
Copy link
Member

appleboy commented Jun 6, 2022

Fixed in #3172

@appleboy appleboy closed this as completed Jun 6, 2022
@appleboy
Copy link
Member

appleboy commented Jun 6, 2022

New released: https://github.com/gin-gonic/gin/releases/tag/v1.8.1

@zetaab
Copy link

zetaab commented Jun 6, 2022

@appleboy to make it clear. What should be the value for feature flag to have same behaviour than it was before 1.8.0? False or True?

@jerome-laforge
Copy link
Contributor Author

jerome-laforge commented Jun 6, 2022

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

No branches or pull requests

5 participants