timeout
timeout copied to clipboard
fix(response): conflict when handler completed and concurrent map writes
Resolve the #15 fatal error: concurrent map writes issue
Cause
In the current implementation, when a timeout occurs, c.Writer is replaced with &gin.responseWriter and writing to the Body is performed by the Timeout.response handler.
Subsequently, additional writes to the Body are performed by the handler being executed in a goroutine.
This results in writes being performed to the Body in a duplicate manner.
Countermeasure
Before replacing c.Writer, use *gin.Context.Copy() to duplicate the context. This ensures that c.Writer is only set to &gin.responseWriter when Timeout.response handler is executed.
Verification
Execute the following verification code.
func main() {
r := gin.New()
r.GET(
"/timeout",
timeout.New(timeout.WithTimeout(200*time.Microsecond),
timeout.WithHandler(func(c *gin.Context) {
time.Sleep(150 * time.Microsecond)
c.JSON(http.StatusOK, gin.H{"code": 200, "data": ""})
}),
timeout.WithResponse(func(c *gin.Context) {
c.JSON(http.StatusRequestTimeout, gin.H{"code": 408, "message": "test"})
})),
)
if err := r.Run(":8080"); err != nil {
log.Fatal(err)
}
}
Request 20 times consecutively using the following command.
for i in {1..20} ; do curl localhost:8080/timeout; echo ""; done
Before the fix, there were instances of duplicated Body content. Additionally, the application occasionally encountered a fatal error: concurrent map writes and would stop.
% for i in {1..20} ; do curl localhost:8080/timeout; echo ""; done
{"code":200,"data":""}
{"code":408,"message":"test"}
{"code":408,"message":"test"}{"code":200,"data":""}
{"code":408,"message":"test"}{"code":200,"data":""}
{"code":200,"data":""}
{"code":200,"data":""}{"code":408,"message":"test"}
{"code":408,"message":"test"}
{"code":408,"message":"test"}
{"code":200,"data":""}
{"code":408,"message":"test"}
{"code":408,"message":"test"}
{"code":408,"message":"test"}
{"code":408,"message":"test"}
{"code":200,"data":""}
{"code":408,"message":"test"}{"code":200,"data":""}
curl: (52) Empty reply from server
curl: (7) Failed to connect to localhost port 8080 after 7 ms: Couldn't connect to server
curl: (7) Failed to connect to localhost port 8080 after 6 ms: Couldn't connect to server
curl: (7) Failed to connect to localhost port 8080 after 7 ms: Couldn't connect to server
curl: (7) Failed to connect to localhost port 8080 after 7 ms: Couldn't connect to server
After the fix, there is no duplication of the Body, and the application does not encounter a fatal error: concurrent map writes that would cause it to stop.
{"code":200,"data":""}
{"code":200,"data":""}
{"code":200,"data":""}
{"code":408,"message":"test"}
{"code":408,"message":"test"}
{"code":408,"message":"test"}
{"code":408,"message":"test"}
{"code":408,"message":"test"}
{"code":408,"message":"test"}
{"code":408,"message":"test"}
{"code":408,"message":"test"}
{"code":408,"message":"test"}
{"code":408,"message":"test"}
{"code":200,"data":""}
{"code":408,"message":"test"}
{"code":408,"message":"test"}
{"code":408,"message":"test"}
{"code":200,"data":""}
{"code":408,"message":"test"}
{"code":408,"message":"test"}
Hello, I try as your code, and test failed.
func (w *Writer) Header() http.Header {
for i := 0; i < 100000; i++ {
w.headers["a"] = []string{"b"}
}
return w.headers
}
There is still a possibility of concurrency.
When I use for {} write , It's 100% present. So must has 2 goroutine parallel operation
@x-lambda I'd like to test your code. Could you please provide more specific instructions on the testing process?
@demouth Reproduce the testing code
func TestLargeResponse(t *testing.T) {
r := gin.New()
r.GET("/slow", New(
WithTimeout(1*time.Second),
WithHandler(func(c *gin.Context) {
c.Next()
}),
WithResponse(func(c *gin.Context) {
c.String(http.StatusRequestTimeout, `{"error": "timeout error"}`)
}),
), func(c *gin.Context) {
time.Sleep(999*time.Millisecond + 500*time.Microsecond) // wait almost same as timeout
c.String(http.StatusRequestTimeout, `{"error": "handler error"}`)
})
wg := sync.WaitGroup{}
for i := 0; i < 100; i++ {
wg.Add(1)
go func() {
defer wg.Done()
w := httptest.NewRecorder()
req, _ := http.NewRequestWithContext(context.Background(), "GET", "/slow", nil)
r.ServeHTTP(w, req)
assert.Equal(t, http.StatusRequestTimeout, w.Code)
assert.Equal(t, `{"error": "timeout error"}`, w.Body.String())
}()
}
wg.Wait()
}
@demouth Any progress?
@appleboy I don't have any good ideas at the moment.
What's the reason that this PR can not yet be merged?