fix broken context.Context implementation

[ekle]

The current implementation of gin.Context does satisfy the context.Context interface, but it returns only default/nil values.

This MR changes this to use the values from the underlying http.Request instead.

This allows for example to use opencensus middleware for cross service tracing which uses the context to transfer tracing-IDs.

[codecov]

Codecov Report

Merging #2029 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #2029   +/-   ##
=======================================
  Coverage   98.69%   98.69%           
=======================================
  Files          40       40           
  Lines        2225     2225           
=======================================
  Hits         2196     2196           
  Misses         16       16           
  Partials       13       13

Impacted Files	Coverage Δ
context.go	98.44% <100%> (ø)	⬆️

---

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 9a820cf...2d85e33. Read the comment docs.

[segevfiner]

Wasn't this done before and was broken due to Gin caching the gin.Context objects?

[ekle]

@segevfiner the current master contains just stubs for the context functions.

gin/context.go

Lines 1033 to 1073 in 6ece26c

	/************************************/
	/***** GOLANG.ORG/X/NET/CONTEXT *****/
	/************************************/
	// Deadline returns the time when work done on behalf of this context
	// should be canceled. Deadline returns ok==false when no deadline is
	// set. Successive calls to Deadline return the same results.
	func (c *Context) Deadline() (deadline time.Time, ok bool) {
	return
	}
	// Done returns a channel that's closed when work done on behalf of this
	// context should be canceled. Done may return nil if this context can
	// never be canceled. Successive calls to Done return the same value.
	func (c *Context) Done() <-chan struct{} {
	return nil
	}
	// Err returns a non-nil error value after Done is closed,
	// successive calls to Err return the same error.
	// If Done is not yet closed, Err returns nil.
	// If Done is closed, Err returns a non-nil error explaining why:
	// Canceled if the context was canceled
	// or DeadlineExceeded if the context's deadline passed.
	func (c *Context) Err() error {
	return nil
	}
	// Value returns the value associated with this context for key, or nil
	// if no value is associated with key. Successive calls to Value with
	// the same key returns the same result.
	func (c *Context) Value(key interface{}) interface{} {
	if key == 0 {
	return c.Request
	}
	if keyAsString, ok := key.(string); ok {
	val, _ := c.Get(keyAsString)
	return val
	}
	return nil
	}

The Value function is just a mapping to some gin internal store, but not to the request context.

[segevfiner]

Yeah I know. I remember looking into this myself and seeing that there was a reverted PR for it because it caused some bug due to gin.Context cache or something like that. #1736

[ekle]

I looked at the #1736
The revert was wrong. It worked perfectly fine.
The panic did just happen because they missed to call the context.Copy function.
Which is a very unusual requirement for context.Context but the gin documentation does say so.

gin/context.go

Lines 89 to 91 in 6ece26c

	// Copy returns a copy of the current context that can be safely used outside the request's scope.
	// This has to be used when the context has to be passed to a goroutine.
	func (c *Context) Copy() *Context {

[bbiao]

@ekle How to avoid this bug? #1731

[bbiao]

@ekle How to avoid this bug? #1731

[ekle]

@bbiao it should not happen if Copy() is called, as stated in the documentation.
If it still would panic, either Copy() is not called or Copy() has a bug.

btw: not calling Copy() in such cases is a big security hole, as the pool would replace the underlying http.Request by reusing the gin.Context while still another instances would access it. The old instance would get access to a different new http.Request. So if it panics, it should, as this could be a big security problem. (the panic is only triggered in some cases)

[bryanpaluch]

Running into this still, and its because the standard library creates a go routine when context.WithTimeout or context.WithDeadline is used. I can work around by spamming c.Copy() all over the place whenever I pass a *gin.Context to a function that may call context.WithTimeout but its a pretty lousy solution when all of my code will likely call a library that uses context.WithTimeout.

Here's a breaking test.

func TestConcurrentHandleContextCancel(t *testing.T) {
	router := New()
	router.GET("/example", func(c *Context) {
		ctx, cancel := context.WithTimeout(c, 1*time.Millisecond)
		defer cancel()
		<-ctx.Done()
		c.String(http.StatusOK, "it worked")
	})

	var wg sync.WaitGroup
	iterations := 200
	wg.Add(iterations)
	for i := 0; i < iterations; i++ {
		go func() {
			req, err := http.NewRequest("GET", "/example", nil)
			assert.NoError(t, err)
			ctx, cancel := context.WithCancel(req.Context())
			req = req.WithContext(ctx)
			w := httptest.NewRecorder()
			router.ServeHTTP(w, req)
			go func() {
				cancel()
			}()
			wg.Done()
		}()
	}
	wg.Wait()
}

If you use c.Copy() on the context.WithTimeout line it won't panic. I can't imagine using c.Copy() all over the place is a good idea, we might as well remove Context pooling. I'm trying to work out middle ground patch.

Chi has had this issue as well,
go-chi/chi#74
go-chi/chi#75