Skip to content

fix: preserve timeout without pooled context reuse#222

Open
hwbrzzl wants to merge 3 commits into
masterfrom
bowen/fix-timeout-pool-race
Open

fix: preserve timeout without pooled context reuse#222
hwbrzzl wants to merge 3 commits into
masterfrom
bowen/fix-timeout-pool-race

Conversation

@hwbrzzl
Copy link
Copy Markdown
Contributor

@hwbrzzl hwbrzzl commented May 21, 2026

Summary

  • Keep automatic request timeouts while replacing the unsafe custom timeout runner with gin-contrib/timeout internally.
  • Preserve 408 Request Timeout responses while preventing late writes from a timed-out request from leaking into the next request.
  • Add regression coverage for timeout completion ordering and request isolation after a timeout.

Closes goravel/goravel#966

Why

The previous timeout middleware enforced timeouts by running the Gin chain in a background goroutine and returning as soon as the deadline fired. That made Goravel return pooled request and response wrappers before the timed-out handler had actually stopped, which could let late writes bleed into a later request.

This change keeps the timeout feature instead of removing it. Goravel still exposes the same Timeout(...) middleware and still returns HTTP 408 on timeout, but the implementation now delegates to gin-contrib/timeout, which buffers the response, suppresses late writes after timeout, and waits for the worker goroutine to finish before control returns to Gin's pooled context lifecycle.

facades.Route().Get("/ping", func(ctx http.Context) http.Response {
	select {
	case <-time.After(5 * time.Second):
		return ctx.Response().Success().Json(http.Json{
			"code":    0,
			"message": "OK",
			"data":    []any{},
			"errors":  []any{},
		})
	case <-ctx.Done():
		return nil
	}
})

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adjusts the Gin driver’s timeout middleware to avoid running the request chain in a background goroutine, preventing pooled context/request/response wrappers from being reused while downstream handlers are still executing (fixing cross-request write contamination after timeouts).

Changes:

  • Run the timeout middleware’s request chain synchronously to avoid use-after-pool-reuse.
  • Ensure ctx.Done(), ctx.Err(), and ctx.Deadline() reflect the request context installed by the timeout middleware.
  • Add regression tests covering a timed-out request followed by a normal request to ensure late writes don’t leak across requests.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
middleware_timeout.go Removes background goroutine execution; runs Next() synchronously after installing a timed context.
middleware_timeout_test.go Adds concurrency/regression tests validating timeout observation and isolation across requests.
context.go Routes Deadline/Done/Err through the request context to reflect middleware-installed context.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread context.go
Comment thread middleware_timeout.go Outdated
Comment thread middleware_timeout_test.go
@hwbrzzl hwbrzzl changed the title fix: avoid pooled context reuse after timeout fix: preserve timeout without pooled context reuse May 23, 2026
Comment thread context.go
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the changes still needed?

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

Successfully merging this pull request may close these issues.

高并发下,某些请求会得到多个响应结果

2 participants