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.

Sign up for GitHub

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

[Heartbeat] Read entire body before closing connection #8660

Merged
merged 3 commits into from
Oct 23, 2018
Merged

[Heartbeat] Read entire body before closing connection #8660

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b5d1e79
[Heartbeat] Read entire body before closing connection
andrewvc Oct 19, 2018
7b5d908
Document/Rename SizedResponseHandler
andrewvc Oct 19, 2018
8a2ff56
Properly cleanup conns in error states in heartbeat
andrewvc Oct 22, 2018
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
15 changes: 15 additions & 0 deletions heartbeat/hbtest/hbtestutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"net/url"
"os"
"strconv"
"strings"
"testing"

"github.com/stretchr/testify/require"
Expand All @@ -51,6 +52,20 @@ func HelloWorldHandler(status int) http.HandlerFunc {
)
}

func LargeResponseHandler(bytes int) http.HandlerFunc {
Copy link

Choose a reason for hiding this comment

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

exported function LargeResponseHandler should have comment or be unexported

var body strings.Builder
for i := 0; i < bytes; i++ {
body.WriteString("x")
}

return http.HandlerFunc(
func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(200)
io.WriteString(w, body.String())
},
)
}

// ServerPort takes an httptest.Server and returns its port as a uint16.
func ServerPort(server *httptest.Server) (uint16, error) {
u, err := url.Parse(server.URL)
Expand Down
35 changes: 35 additions & 0 deletions heartbeat/monitors/active/http/http_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,41 @@ func TestDownStatuses(t *testing.T) {
}
}

func TestLargeResponse(t *testing.T) {
server := httptest.NewServer(hbtest.LargeResponseHandler(1024 * 1024))
defer server.Close()

configSrc := map[string]interface{}{
"urls": server.URL,
"timeout": "1s",
"check.response.body": "x",
}

config, err := common.NewConfigFrom(configSrc)
require.NoError(t, err)

jobs, err := create("largeresp", config)
require.NoError(t, err)

job := jobs[0]

event, _, err := job.Run()
require.NoError(t, err)

port, err := hbtest.ServerPort(server)
require.NoError(t, err)

mapvaltest.Test(
t,
mapval.Strict(mapval.Compose(
hbtest.MonitorChecks("http@"+server.URL, server.URL, "127.0.0.1", "http", "up"),
hbtest.RespondingTCPChecks(port),
respondingHTTPChecks(server.URL, 200),
)),
event.Fields,
)
}

func TestHTTPSServer(t *testing.T) {
server := httptest.NewTLSServer(hbtest.HelloWorldHandler(http.StatusOK))
port, err := hbtest.ServerPort(server)
Expand Down
18 changes: 17 additions & 1 deletion heartbeat/monitors/active/http/simple_transp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ func (t *SimpleTransport) RoundTrip(req *http.Request) (*http.Response, error) {
if err != nil {
return nil, err
}
defer conn.Close()

requestedGzip := false
if t.DisableCompression &&
Expand Down Expand Up @@ -147,13 +146,30 @@ func (t *SimpleTransport) writeRequest(conn net.Conn, req *http.Request) error {
return err
}

// comboConnReadCloser wraps a ReadCloser that is backed by
// on a net.Conn. It will close the net.Conn when the ReadCloser is closed.
type comboConnReadCloser struct {
conn net.Conn
rc io.ReadCloser
}

func (c comboConnReadCloser) Read(p []byte) (n int, err error) {
return c.rc.Read(p)
}

func (c comboConnReadCloser) Close() error {
defer c.conn.Close()
return c.rc.Close()
}

func (t *SimpleTransport) readResponse(
conn net.Conn,
req *http.Request,
requestedGzip bool,
) (*http.Response, error) {
reader := bufio.NewReader(conn)
resp, err := http.ReadResponse(reader, req)
resp.Body = comboConnReadCloser{conn, resp.Body}
Copy link
Contributor

Choose a reason for hiding this comment

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

When exactly is the Body now closed? I see below that the body is closed on error, but what about the standard case?

If I understand it right the connection is closed as soon as the body is closed.

Copy link

Choose a reason for hiding this comment

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

The Body MUST always be closed once processing has finished.

From http docs:

The client must close the response body when finished with it:

resp, err := http.Get("http://example.com/")
if err != nil {
	// handle error
}
defer resp.Body.Close()
body, err := ioutil.ReadAll(resp.Body)
// ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, @urso has it right. It's up to the user of the HTTP client to always close the body.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To add some clarity, it's a bit weird in terms of design. You'd think that you would call resp.Close(), which would implicitly close the body, but that's not the design choice they made. I can see why, since it's usually the only resource.

Normally the connection lifecycle is decoupled from the body lifecycle to enable things like keepalive and connection pooling. In our case we want them 100% coupled, so we have this awkward dance we do with the combination closer.

Copy link
Contributor

Choose a reason for hiding this comment

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

When exactly is the Body now closed? I see below that the body is closed on error, but what about the standard case?

If I understand it right the connection is closed as soon as the body is closed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above comment.

if err != nil {
return nil, err
}
Expand Down