timeout icon indicating copy to clipboard operation
timeout copied to clipboard

fix(response): conflict when handler completed and concurrent map writes

Open demouth opened this issue 2 years ago • 6 comments

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"}

demouth avatar Sep 08 '23 23:09 demouth

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 avatar Sep 19 '23 07:09 x-lambda

@x-lambda I'd like to test your code. Could you please provide more specific instructions on the testing process?

demouth avatar Oct 14 '23 14:10 demouth

@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()
}

appleboy avatar Nov 25 '23 03:11 appleboy

@demouth Any progress?

appleboy avatar Feb 18 '24 02:02 appleboy

@appleboy I don't have any good ideas at the moment.

demouth avatar Aug 29 '24 08:08 demouth

What's the reason that this PR can not yet be merged?

zhyee avatar Sep 02 '24 09:09 zhyee