gin-gonic / gin

Gin is a HTTP web framework written in Go (Golang). It features a Martini-like API with much better performance -- up to 40 times faster. If you need smashing performance, get yourself some Gin.
https://gin-gonic.com/
MIT License
78.94k stars 8.02k forks source link

Context Copy does not detach request.context #3554

Open taehwoi opened 1 year ago

taehwoi commented 1 year ago

Description

It seems like we can use context.Copy() to detach gin context from the request's scope. Or at least that is what the docs suggest, regarding using Copy() to pass context to a goroutine. However the Copy() method reuses the pointer to c.Request, so the copied context is still cancelled by original request's cancellation.

I think Copy() should do something comparable to the newly added https://pkg.go.dev/context@master#WithoutCancel, preserving the values but removing deadline and cancel propagation of the parent context.

How to reproduce

package main

import (
        "fmt"
        "time"

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

func main() {
    g := gin.Default()
        g.ContextWithFallback = true
    g.GET("/", func(c *gin.Context) {

        go func(c *gin.Context) {
            <-c.Done()
            fmt.Println("ctx1 cancelled")
        }(c)

        go func(c *gin.Context) {
            <-c.Done()
            fmt.Println("ctx2 cancelled")
        }(c.Copy())

                time.Sleep(time.Minute)
    })
    g.Run(":9000")
}

Expectations

$ curl -m 3 http://localhost:9000

only ctx1 cancelled should be printed

ctx1 cancelled

Actual result

$ curl -m 3 http://localhost:9000

the copied context is cancelled as well

ctx1 cancelled
ctx2 cancelled

Environment

Tseian commented 1 year ago

The go.Context.Done method is implemented as follows.

// Done returns nil (chan which will wait forever) when c.Request has no Context.
func (c *Context) Done() <-chan struct{} {
    if !c.engine.ContextWithFallback || c.Request == nil || c.Request.Context() == nil {
        return nil
    }
    return c.Request.Context().Done()
}

So I think you should set ContextWithFallback to false to meet expectations.

type Engine struct {
    // ....
    // ContextWithFallback enable fallback Context.Deadline(), Context.Done(), Context.Err() and Context.Value() when Context.Request.Context() is not nil.
    ContextWithFallback bool
        // ....
}
taehwoi commented 1 year ago

The go.Context.Done method is implemented as follows.

// Done returns nil (chan which will wait forever) when c.Request has no Context.
func (c *Context) Done() <-chan struct{} {
  if !c.engine.ContextWithFallback || c.Request == nil || c.Request.Context() == nil {
      return nil
  }
  return c.Request.Context().Done()
}

So I think you should set ContextWithFallback to false to meet expectations.

type Engine struct {
  // ....
  // ContextWithFallback enable fallback Context.Deadline(), Context.Done(), Context.Err() and Context.Value() when Context.Request.Context() is not nil.
  ContextWithFallback bool
        // ....
}

@Tseian Thanks for the suggestion. But setting ContextWithFallback = false returns a nil channel for both c.Done()and c.Copy().Done(). So neither c, nor c.Copy() is cancelled.

I was hoping that with ContextWithFallback = true only c would be cancelled on request cancel.

omgupta1608 commented 1 month ago

I was hoping that with ContextWithFallback = true only c would be cancelled on request cancel.

@taehwoi

Thanks @Tseian Yes, I was expecting the same. Though setting ContextWithFallback to false doesn't work.