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

Read response when client closes connection #1232 #1233

Merged
merged 8 commits into from Mar 14, 2022

Conversation

ArminBTVS
Copy link
Contributor

@ArminBTVS ArminBTVS commented Mar 3, 2022

Closes #1232

Copy link
Collaborator

@erikdubbelboer erikdubbelboer left a comment

Can you rebase your pull request on master.

@ArminBTVS ArminBTVS force-pushed the forcefully-close-conn-resp branch from 6d164c0 to 6217429 Compare Mar 4, 2022
@ArminBTVS ArminBTVS requested a review from erikdubbelboer Mar 4, 2022
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)
Copy link
Collaborator

@erikdubbelboer erikdubbelboer Mar 6, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@moredure moredure Mar 6, 2022

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

@ArminBTVS ArminBTVS Mar 7, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@moredure moredure Mar 7, 2022

Choose a reason for hiding this comment

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

Ack

http.go Outdated
@@ -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) {
Copy link
Contributor

@moredure moredure Mar 6, 2022

Choose a reason for hiding this comment

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

Can be embedded into next if err != nil block

Copy link
Contributor Author

@ArminBTVS ArminBTVS Mar 7, 2022

Choose a reason for hiding this comment

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

Did change this

Copy link
Contributor

@moredure moredure Mar 7, 2022

Choose a reason for hiding this comment

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

@ArminBTVS please check latest comments

http.go Outdated
if err != nil {
return err
}
}

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

@moredure moredure Mar 6, 2022

Choose a reason for hiding this comment

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

Can be embedded into next if err != nil block

Copy link
Contributor Author

@ArminBTVS ArminBTVS Mar 7, 2022

Choose a reason for hiding this comment

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

Did change this

http.go Outdated
return nil
}
if err != nil {
if err != nil && !errors.Is(err, syscall.ECONNRESET) {
Copy link
Contributor

@moredure moredure Mar 7, 2022

Choose a reason for hiding this comment

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

It's a little bit not what I meant,

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

http.go Outdated
return nil
}
if err != nil && err != io.EOF {
if err != nil && err != io.EOF && !errors.Is(err, syscall.ECONNRESET) {
Copy link
Contributor

@moredure moredure Mar 7, 2022

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

@ArminBTVS ArminBTVS Mar 7, 2022

Choose a reason for hiding this comment

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

@moredure OK. Changed to your recomendation.

@ArminBTVS
Copy link
Contributor Author

ArminBTVS commented Mar 7, 2022

@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

@ArminBTVS ArminBTVS force-pushed the forcefully-close-conn-resp branch from c47259b to 4e9614f Compare Mar 7, 2022
@ArminBTVS ArminBTVS requested a review from erikdubbelboer Mar 7, 2022
client_test.go Outdated Show resolved Hide resolved
client_test.go Outdated Show resolved Hide resolved
client_test.go Outdated Show resolved Hide resolved
client_test.go Outdated Show resolved Hide resolved
client_test.go Outdated Show resolved Hide resolved
client_test.go Outdated Show resolved Hide resolved
client_test.go Outdated Show resolved Hide resolved
client_test.go Outdated Show resolved Hide resolved
client_test.go Outdated Show resolved Hide resolved
client_test.go Outdated Show resolved Hide resolved
@ArminBTVS ArminBTVS requested a review from erikdubbelboer Mar 8, 2022
@erikdubbelboer
Copy link
Collaborator

erikdubbelboer commented Mar 8, 2022

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
Copy link
Contributor Author

ArminBTVS commented Mar 9, 2022

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
Copy link
Contributor Author

ArminBTVS commented Mar 11, 2022

@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
Copy link
Collaborator

erikdubbelboer commented Mar 13, 2022

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 erikdubbelboer merged commit 1a5f2f4 into valyala:master Mar 14, 2022
11 checks passed
@erikdubbelboer
Copy link
Collaborator

erikdubbelboer commented Mar 14, 2022

Thanks!

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.

3 participants