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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

demouth
Copy link
Contributor

@demouth demouth commented Sep 8, 2023

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

@x-lambda
Copy link

x-lambda commented Sep 19, 2023

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

@demouth
Copy link
Contributor Author

demouth commented Oct 14, 2023

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

@appleboy
Copy link
Member

@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 appleboy added the bug Something isn't working label Nov 25, 2023
@appleboy appleboy changed the title fix fatal error: concurrent map writes fix(response): conflict when handler completed and concurrent map writes Nov 25, 2023
@appleboy
Copy link
Member

@demouth Any progress?

james-storey pushed a commit to james-storey/timeout that referenced this pull request Mar 19, 2024
@demouth
Copy link
Contributor Author

demouth commented Aug 29, 2024

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

@zhyee
Copy link
Contributor

zhyee commented Sep 2, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants