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

Comment From: jerome-laforge

In my humble opinion, the root cause of this issue is https://github.com/gin-gonic/gin/pull/2769

Comment From: zetaab

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

Comment From: wei840222

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

Comment From: jerome-laforge

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

Comment From: appleboy

@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.

Comment From: thinkerou

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

Comment From: appleboy

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

Comment From: wei840222

@appleboy Yes I can

Comment From: wei840222

Hi, @appleboy here is the pr https://github.com/gin-gonic/gin/pull/3172

Comment From: appleboy

Fixed in #3172

Comment From: appleboy

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

Comment From: zetaab

@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?

Comment From: jerome-laforge

False and since it is default value. So you don't need to change anything in your code base in order to have the same behavior of pre v1.8.0

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration, Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law; they should not be distributed, used or copied without authorisation. If you have received this email in error, please notify the sender and delete this message and its attachments. As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified. Thank you.

Comment From: kbolino

Though this has been fixed, I personally think the OP is not using context properly.

If you kick off a background task running beyond the lifetime of the HTTP request being served, then it should get its own context rooted at context.Background() (the name of which is rather appropriate here) and not re-use the request's context, whether that be the gin.Context or the one attached to the http.Request.