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

The operation of "timeout" should be removed because it may lead to multiple handlers using the same gin.Context simultaneously, thus causing incorrect response results. #67

Open
skyterra opened this issue May 8, 2024 · 1 comment

Comments

@skyterra
Copy link

skyterra commented May 8, 2024

// New wraps a handler and aborts the process of the handler if the timeout is reached
func New(opts ...Option) gin.HandlerFunc {
	t := &Timeout{
		timeout:  defaultTimeout,
		handler:  nil,
		response: defaultResponse,
	}

	// Loop through each option
	for _, opt := range opts {
		if opt == nil {
			panic("timeout Option not be nil")
		}

		// Call the option giving the instantiated
		opt(t)
	}

	if t.timeout <= 0 {
		return t.handler
	}

	bufPool = &BufferPool{}

	return func(c *gin.Context) {
		finish := make(chan struct{}, 1)
		panicChan := make(chan interface{}, 1)

		w := c.Writer
		buffer := bufPool.Get()
		tw := NewWriter(w, buffer)
		c.Writer = tw
		buffer.Reset()

		go func() {
			defer func() {
				if p := recover(); p != nil {
					panicChan <- p
				}
			}()
			t.handler(c)
			finish <- struct{}{}
		}()

		select {
		case p := <-panicChan:
			tw.FreeBuffer()
			c.Writer = w
			panic(p)

		case <-finish:
			c.Next()
			tw.mu.Lock()
			defer tw.mu.Unlock()
			dst := tw.ResponseWriter.Header()
			for k, vv := range tw.Header() {
				dst[k] = vv
			}

			if _, err := tw.ResponseWriter.Write(buffer.Bytes()); err != nil {
				panic(err)
			}
			tw.FreeBuffer()
			bufPool.Put(buffer)

		case <-time.After(t.timeout): 
			c.Abort()
			tw.mu.Lock()
			defer tw.mu.Unlock()
			tw.timeout = true
			tw.FreeBuffer()
			bufPool.Put(buffer)

			c.Writer = w
			t.response(c)
			c.Writer = tw
		}
	}
}

case <-time.After(t.timeout): c.Abort()

If a timeout occurs and c.Abort() is called here, but the handler function has not completed execution (for example, waiting for database data to be read), it will still continue to execute. In such a case, when c.PureJSON() is called in the handler function, c may have already been used by subsequent requests.

so, i suggest remove timeout operation.

@willthrom
Copy link

willthrom commented Oct 23, 2024

Isn't this a serious issue?

We are having a weird situation where this might be the issue.
We end with a request being responded with TWO json, where the first JSON seems to come from a previous request.

I can say thought the first request failed with a context cancel, and the second request when through successfully but the final response JSON seems to have, as mentioned, both message!

I don't see though how come the context could be shared between two request (I might not seeing in the library code..), even thought that seems exactly what it is happening!

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

No branches or pull requests

2 participants