Read response when client closes connection #1232 #1233

ArminBTVS · 2022-03-03T14:25:44Z

erikdubbelboer

Can you rebase your pull request on master.

erikdubbelboer · 2022-03-06T12:32:12Z

client.go

 		c.closeConn(cc)
 		// Don't retry in case of ErrBodyTooLarge since we will just get the same again.
 		retry := err != ErrBodyTooLarge
 		return retry, err
 	}
-	c.releaseReader(br)

 	if resetConnection || req.ConnectionClose() || resp.ConnectionClose() {
 		c.closeConn(cc)
 	} else {
 		c.releaseConn(cc)


Now when a connection is closed with ECONNRESET it will still be released to the pool here (as there was no error before) and will try to be used again.

@ArminBTVS, thanks for your pull request

c.releaseWriter(bw) isECONNRESET := errors.Is(err, syscall.ECONNRESET) if err != nil && !isECONNRESET {

if resetConnection || req.ConnectionClose() || resp.ConnectionClose() || isECONNRESET {

needed

@moredure true. Did not see this case. I did make the change as @moredure suggested. Thanks!

moredure · 2022-03-06T20:55:32Z

http.go

@@ -1290,13 +1291,19 @@ func (resp *Response) ReadLimitBody(r *bufio.Reader, maxBodySize int) error {

 	if !resp.mustSkipBody() {
 		err = resp.ReadBody(r, maxBodySize)
+		if errors.Is(err, syscall.ECONNRESET) {


Can be embedded into next if err != nil block

Did change this

@ArminBTVS please check latest comments

moredure · 2022-03-06T20:55:42Z

http.go

 		if err != nil {
 			return err
 		}
 	}

 	if resp.Header.ContentLength() == -1 {
 		err = resp.Header.ReadTrailer(r)
+		if errors.Is(err, syscall.ECONNRESET) {


Can be embedded into next if err != nil block

Did change this

moredure · 2022-03-07T08:15:37Z

http.go

-			return nil
-		}
-		if err != nil {
+		if err != nil && !errors.Is(err, syscall.ECONNRESET) {


It's a little bit not what I meant,

if err != nil { if errors.Is(err, syscall.ECONNRESET) { return nil } return err }

moredure · 2022-03-07T08:15:56Z

http.go

-			return nil
-		}
-		if err != nil && err != io.EOF {
+		if err != nil && err != io.EOF && !errors.Is(err, syscall.ECONNRESET) {


same as above

@moredure OK. Changed to your recomendation.

ArminBTVS · 2022-03-07T08:55:11Z

@erikdubbelboer @moredure It seems like the behavior on windows is different than on unix. My test never fails on linux even when I run it for multiple times, but never passes on windows. I will further investigate the issue using an windows OS.

Cause for failure is a different sycall response for connection reset under windows. Fixed by checking for OS specific error code

client_test.go

erikdubbelboer · 2022-03-08T17:06:17Z

Looks like the tests are still failing on windows:

--- FAIL: TestRstConnResponseWhileSending (0.00s)
    client_test.go:2898: the server closed connection before returning the first response byte. Make sure the server returns 'Connection: close' response header before closing the connection

ArminBTVS · 2022-03-09T12:15:51Z

Looks like the tests are still failing on windows:

--- FAIL: TestRstConnResponseWhileSending (0.00s)
    client_test.go:2898: the server closed connection before returning the first response byte. Make sure the server returns 'Connection: close' response header before closing the connection

It seems timing dependent. Bad!

Was able to reproduce this behavior on windows only when running the tests for 100 times in a loop. I will further look into it.

ArminBTVS · 2022-03-11T07:56:17Z

@erikdubbelboer so I further investigated on the failing test on Windows. When I repeat the test for >100 times it will fail. But only on Windows. It does not happen on Linux. The issue goes all the way down to the actual syscalls.

So what happens on Windows is that even after I called _, err = conn.Write([]byte("HTTP/1.1 418 Teapot\r\n\r\n")) (which used WSASend) in the server go routine, the read of the connection will immediately fail with an connection reset error, even if I closed the connection after conn.Write succeeded without any error.

What I wanted to simulate in the test is the following simplified traffic:

client -> server REQ
client <- server RESP
client <- server RST

But it seems that I am not really able to do so on Windows. I could run the test on Linux only? Or add a sleep between write and forcefully closing the connection? Or do you have any thoughts/ideas on this?

erikdubbelboer · 2022-03-13T08:09:47Z

Could also have to do with using sockets on the same machine instead of between different machines. I have seen TCP behave differently that case other times before (even on Linux). Just ignore the test on windows, we don't have great windows support either way 😅

erikdubbelboer · 2022-03-14T09:53:21Z

Thanks!

erikdubbelboer requested changes Mar 3, 2022

View reviewed changes

ArminBTVS force-pushed the forcefully-close-conn-resp branch from 6d164c0 to 6217429 Compare March 4, 2022 07:29

ArminBTVS requested a review from erikdubbelboer March 4, 2022 13:31

erikdubbelboer requested changes Mar 6, 2022

View reviewed changes

moredure reviewed Mar 6, 2022

View reviewed changes

moredure reviewed Mar 7, 2022

View reviewed changes

becheran added 6 commits March 7, 2022 11:09

Read response when client closes connection valyala#1232

135c2c4

Fix edge case were client responds with invalid header

de67094

Follow linter suggestions for tests

59d2fcc

Changes after review

5c93662

Reafactor error check after review

3b1bcc7

Handle connection reset on windows

4e9614f

ArminBTVS force-pushed the forcefully-close-conn-resp branch from c47259b to 4e9614f Compare March 7, 2022 10:11

ArminBTVS requested a review from erikdubbelboer March 7, 2022 10:23

erikdubbelboer requested changes Mar 7, 2022

View reviewed changes

Remove format string from test where not needed

e4d02bd

ArminBTVS requested a review from erikdubbelboer March 8, 2022 08:44

Run connection reset tests not on Windows

8d7b611

erikdubbelboer merged commit 1a5f2f4 into valyala:master Mar 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read response when client closes connection #1232 #1233

Read response when client closes connection #1232 #1233

ArminBTVS commented Mar 3, 2022

erikdubbelboer left a comment

erikdubbelboer Mar 6, 2022

moredure Mar 6, 2022 •

edited

ArminBTVS Mar 7, 2022

moredure Mar 7, 2022

moredure Mar 6, 2022

ArminBTVS Mar 7, 2022

moredure Mar 7, 2022

moredure Mar 6, 2022

ArminBTVS Mar 7, 2022

moredure Mar 7, 2022

moredure Mar 7, 2022

ArminBTVS Mar 7, 2022

ArminBTVS commented Mar 7, 2022 •

edited

erikdubbelboer commented Mar 8, 2022

ArminBTVS commented Mar 9, 2022

ArminBTVS commented Mar 11, 2022

erikdubbelboer commented Mar 13, 2022

erikdubbelboer commented Mar 14, 2022

Read response when client closes connection #1232 #1233

Read response when client closes connection #1232 #1233

Conversation

ArminBTVS commented Mar 3, 2022

erikdubbelboer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moredure Mar 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArminBTVS commented Mar 7, 2022 • edited

erikdubbelboer commented Mar 8, 2022

ArminBTVS commented Mar 9, 2022

ArminBTVS commented Mar 11, 2022

erikdubbelboer commented Mar 13, 2022

erikdubbelboer commented Mar 14, 2022

moredure Mar 6, 2022 •

edited

ArminBTVS commented Mar 7, 2022 •

edited