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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions client.go
Original file line number Diff line number Diff line change
Expand Up @@ -1437,12 +1437,12 @@ func (c *HostClient) doNonNilReqResp(req *Request, resp *Response) (bool, error)
if err == nil {
err = bw.Flush()
}
if err != nil {
c.releaseWriter(bw)
c.releaseWriter(bw)
isConnRST := isConnectionReset(err)
if err != nil && !isConnRST {
c.closeConn(cc)
return true, err
}
c.releaseWriter(bw)

if c.ReadTimeout > 0 {
// Set Deadline every time, since golang has fixed the performance issue
Expand All @@ -1462,22 +1462,22 @@ func (c *HostClient) doNonNilReqResp(req *Request, resp *Response) (bool, error)
}

br := c.acquireReader(conn)
if err = resp.ReadLimitBody(br, c.MaxResponseBodySize); err != nil {
c.releaseReader(br)
err = resp.ReadLimitBody(br, c.MaxResponseBodySize)
c.releaseReader(br)
if err != nil {
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() {
if resetConnection || req.ConnectionClose() || resp.ConnectionClose() || isConnRST {
c.closeConn(cc)
} else {
c.releaseConn(cc)
Copy link
Collaborator

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

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

Choose a reason for hiding this comment

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

Ack

}

return false, err
return false, nil
}

var (
Expand Down
2 changes: 1 addition & 1 deletion client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2837,6 +2837,6 @@ func TestHttpsRequestWithoutParsedURL(t *testing.T) {

_, err := client.doNonNilReqResp(req, &Response{})
if err != nil {
t.Fatalf("https requests with IsTLS client must succeed")
t.Fatal("https requests with IsTLS client must succeed")
}
}
136 changes: 136 additions & 0 deletions client_unix_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
//go:build !windows
// +build !windows

package fasthttp

import (
"io"
"io/ioutil"
"net"
"net/http"
"strings"
"testing"
)

// See issue #1232
func TestRstConnResponseWhileSending(t *testing.T) {
const expectedStatus = http.StatusTeapot
const payload = "payload"

srv, err := net.Listen("tcp", "127.0.0.1:0")
if err != nil {
t.Fatal(err)
}
defer srv.Close()

go func() {
for {
conn, err := srv.Accept()
if err != nil {
return
}

// Read at least one byte of the header
// Otherwise we would have an unsolicited response
_, err = ioutil.ReadAll(io.LimitReader(conn, 1))
if err != nil {
t.Error(err)
}

// Respond
_, err = conn.Write([]byte("HTTP/1.1 418 Teapot\r\n\r\n"))
if err != nil {
t.Error(err)
}

// Forcefully close connection
err = conn.(*net.TCPConn).SetLinger(0)
if err != nil {
t.Error(err)
}
conn.Close()
}
}()

svrUrl := "http://" + srv.Addr().String()
client := HostClient{Addr: srv.Addr().String()}

for i := 0; i < 100; i++ {
req := AcquireRequest()
defer ReleaseRequest(req)
resp := AcquireResponse()
defer ReleaseResponse(resp)

req.Header.SetMethod("POST")
req.SetBodyStream(strings.NewReader(payload), len(payload))
req.SetRequestURI(svrUrl)

err = client.Do(req, resp)
if err != nil {
t.Fatal(err)
}
if expectedStatus != resp.StatusCode() {
t.Fatalf("Expected %d status code, but got %d", expectedStatus, resp.StatusCode())
}
}
}

// See issue #1232
func TestRstConnClosedWithoutResponse(t *testing.T) {
const payload = "payload"

srv, err := net.Listen("tcp", "127.0.0.1:0")
if err != nil {
t.Fatal(err)
}
defer srv.Close()

go func() {
for {
conn, err := srv.Accept()
if err != nil {
return
}

// Read at least one byte of the header
// Otherwise we would have an unsolicited response
_, err = ioutil.ReadAll(io.LimitReader(conn, 1))
if err != nil {
t.Error(err)
}

// Respond with incomplete header
_, err = conn.Write([]byte("Http"))
if err != nil {
t.Error(err)
}

// Forcefully close connection
err = conn.(*net.TCPConn).SetLinger(0)
if err != nil {
t.Error(err)
}
conn.Close()
}
}()

svrUrl := "http://" + srv.Addr().String()
client := HostClient{Addr: srv.Addr().String()}

for i := 0; i < 100; i++ {
req := AcquireRequest()
defer ReleaseRequest(req)
resp := AcquireResponse()
defer ReleaseResponse(resp)

req.Header.SetMethod("POST")
req.SetBodyStream(strings.NewReader(payload), len(payload))
req.SetRequestURI(svrUrl)

err = client.Do(req, resp)

if !isConnectionReset(err) {
t.Fatal("Expected connection reset error")
}
}
}
6 changes: 6 additions & 0 deletions http.go
Original file line number Diff line number Diff line change
Expand Up @@ -1291,13 +1291,19 @@ func (resp *Response) ReadLimitBody(r *bufio.Reader, maxBodySize int) error {
if !resp.mustSkipBody() {
err = resp.ReadBody(r, maxBodySize)
if err != nil {
if isConnectionReset(err) {
return nil
}
return err
}
}

if resp.Header.ContentLength() == -1 {
err = resp.Header.ReadTrailer(r)
if err != nil && err != io.EOF {
if isConnectionReset(err) {
return nil
}
return err
}
}
Expand Down
13 changes: 13 additions & 0 deletions tcp.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
//go:build !windows
// +build !windows

package fasthttp

import (
"errors"
"syscall"
)

func isConnectionReset(err error) bool {
return errors.Is(err, syscall.ECONNRESET)
}
13 changes: 13 additions & 0 deletions tcp_windows.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
//go:build windows
// +build windows

package fasthttp

import (
"errors"
"syscall"
)

func isConnectionReset(err error) bool {
return errors.Is(err, syscall.WSAECONNRESET)
}