[Heartbeat] Read entire body before closing connection #8660

andrewvc · 2018-10-19T14:01:22Z

This fixes an issue where the connection would be closed after only a partial body read. The RoundTripper would close the conn before returning the response which included a partially buffered body. This would actually work for short responses, since the backing bufio would do a partial read, but would fail on all but the shortest responses.

Normally connection lifecycle is handled outside the realm of the RoundTripper, but for our purposes we don't want to re-use connections. Since it is a requirement that all response bodies be closed, we can piggy-back on top of that to ensure the connection is closed.

@urso I would like your feedback on this fix. If it looks good I'll add some unit tests around it.

Fixes #8588

houndci-bot · 2018-10-19T14:01:30Z

heartbeat/monitors/active/http/simple_transp.go

+	return c.Read(p)
+}
+
+func (c ComboConnReadCloser) Close() error {


exported method ComboConnReadCloser.Close should have comment or be unexported

andrewvc · 2018-10-19T15:04:11Z

Pushed again to fix an oversight. I should mention, a minimal test case is:

heartbeat.monitors:
- type: http
  schedule: '@every 5s'
  urls:
  - https://www.elastic.co
  check.response:
    body: Elasticsearch

This fixes an issue where the connection would be closed after only a partial body read. The RoundTripper would close the conn before returning the response which included a partially buffered body. This would actually work for short responses, since the backing bufio would do a partial read, but would fail on all but the shortest responses. Normally connection lifecycle is handled outside the realm of the `RoundTripper`, but for our purposes we don't want to re-use connections. Since it is a requirement that all response bodies be closed, we can piggy-back on top of that to ensure the connection is closed. Fixes elastic#8588

houndci-bot · 2018-10-19T17:34:38Z

heartbeat/hbtest/hbtestutil.go

@@ -51,6 +52,20 @@ func HelloWorldHandler(status int) http.HandlerFunc {
 	)
 }

+func LargeResponseHandler(bytes int) http.HandlerFunc {


exported function LargeResponseHandler should have comment or be unexported

andrewvc · 2018-10-19T17:35:12Z

Added a test. Confirmed via manual testing that adding back in the removed defer causes the test to go red.

andrewvc · 2018-10-19T21:39:16Z

jenkins, retest this please

ruflin · 2018-10-22T08:44:15Z

heartbeat/monitors/active/http/simple_transp.go

 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}


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.

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) // ...

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

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.

ruflin · 2018-10-22T08:44:36Z

heartbeat/monitors/active/http/simple_transp.go

 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}


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.

See above comment.

urso

Skimming the RoundTrip method, it looks like it could produce even more resource leaks. E.g. when the context is stopped due to timeout, no one will consume the response.

andrewvc · 2018-10-22T22:14:08Z

@urso you're right! The error state would leave a leaked goroutine + response.

I've just pushed a commit that should address this.

ruflin · 2018-10-23T07:51:43Z

jenkins, test this

ruflin · 2018-10-23T07:52:06Z

I added the review and needs_backport label.

This fixes an issue where the connection would be closed after only a partial body read. The RoundTripper would close the conn before returning the response which included a partially buffered body. This would actually work for short responses, since the backing bufio would do a partial read, but would fail on all but the shortest responses. Normally connection lifecycle is handled outside the realm of the `RoundTripper`, but for our purposes we don't want to re-use connections. Since it is a requirement that all response bodies be closed, we can piggy-back on top of that to ensure the connection is closed. (cherry picked from commit 1e03ff4)

andrewvc added bug Heartbeat labels Oct 19, 2018

andrewvc requested a review from urso October 19, 2018 14:01

houndci-bot reviewed Oct 19, 2018

View reviewed changes

andrewvc force-pushed the fix-early-conn-close branch 2 times, most recently from 57b106d to 4873d74 Compare October 19, 2018 15:03

andrewvc force-pushed the fix-early-conn-close branch from 4873d74 to b5d1e79 Compare October 19, 2018 17:34

houndci-bot reviewed Oct 19, 2018

View reviewed changes

Document/Rename SizedResponseHandler

7b5d908

andrewvc mentioned this pull request Oct 19, 2018

JSON Checks for Heartbeat HTTP Monitors #8667

Merged

ruflin reviewed Oct 22, 2018

View reviewed changes

urso reviewed Oct 22, 2018

View reviewed changes

andrewvc force-pushed the fix-early-conn-close branch from 57ad0b0 to 985a5d2 Compare October 22, 2018 22:13

Properly cleanup conns in error states in heartbeat

8a2ff56

andrewvc force-pushed the fix-early-conn-close branch from 985a5d2 to 8a2ff56 Compare October 22, 2018 22:15

ruflin added review needs_backport PR is waiting to be backported to other branches. labels Oct 23, 2018

ruflin approved these changes Oct 23, 2018

View reviewed changes

andrewvc merged commit 1e03ff4 into elastic:master Oct 23, 2018

andrewvc mentioned this pull request Oct 23, 2018

Cherry-pick #8660 to 6.x: [Heartbeat] Read entire body before closing connection #8715

Merged

andrewvc removed the needs_backport PR is waiting to be backported to other branches. label Oct 23, 2018

andrewvc added the v6.5.0 label Oct 23, 2018

andrewvc deleted the fix-early-conn-close branch October 31, 2018 22:14

andrewvc mentioned this pull request Nov 9, 2018

Cherry-pick #8667 to 6.x: JSON Checks for Heartbeat HTTP Monitors #9015

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Heartbeat] Read entire body before closing connection #8660

[Heartbeat] Read entire body before closing connection #8660

andrewvc commented Oct 19, 2018

houndci-bot Oct 19, 2018

andrewvc commented Oct 19, 2018 •

edited

Loading

houndci-bot Oct 19, 2018

andrewvc commented Oct 19, 2018

andrewvc commented Oct 19, 2018

ruflin Oct 22, 2018

urso Oct 22, 2018

andrewvc Oct 22, 2018

andrewvc Oct 22, 2018

ruflin Oct 22, 2018

andrewvc Oct 22, 2018

urso left a comment

andrewvc commented Oct 22, 2018

ruflin commented Oct 23, 2018

ruflin commented Oct 23, 2018

[Heartbeat] Read entire body before closing connection #8660

[Heartbeat] Read entire body before closing connection #8660

Conversation

andrewvc commented Oct 19, 2018

Choose a reason for hiding this comment

andrewvc commented Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

andrewvc commented Oct 19, 2018

andrewvc commented Oct 19, 2018

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

urso left a comment

Choose a reason for hiding this comment

andrewvc commented Oct 22, 2018

ruflin commented Oct 23, 2018

ruflin commented Oct 23, 2018

andrewvc commented Oct 19, 2018 •

edited

Loading