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

[Heartbeat] Read entire body before closing connection #8660

Merged
merged 3 commits into from
Oct 23, 2018

Conversation

andrewvc
Copy link
Contributor

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

return c.Read(p)
}

func (c ComboConnReadCloser) Close() error {

Choose a reason for hiding this comment

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

exported method ComboConnReadCloser.Close should have comment or be unexported

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

andrewvc commented Oct 19, 2018

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
@andrewvc andrewvc force-pushed the fix-early-conn-close branch from 4873d74 to b5d1e79 Compare October 19, 2018 17:34
@@ -51,6 +52,20 @@ func HelloWorldHandler(status int) http.HandlerFunc {
)
}

func LargeResponseHandler(bytes int) http.HandlerFunc {

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

@andrewvc
Copy link
Contributor Author

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

@andrewvc
Copy link
Contributor Author

jenkins, retest this please

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.

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

Copy link

@urso urso left a comment

Choose a reason for hiding this comment

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

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 andrewvc force-pushed the fix-early-conn-close branch from 57ad0b0 to 985a5d2 Compare October 22, 2018 22:13
@andrewvc
Copy link
Contributor Author

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

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

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

ruflin commented Oct 23, 2018

jenkins, test this

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

ruflin commented Oct 23, 2018

I added the review and needs_backport label.

@andrewvc andrewvc merged commit 1e03ff4 into elastic:master Oct 23, 2018
andrewvc added a commit to andrewvc/beats that referenced this pull request Oct 23, 2018
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 andrewvc removed the needs_backport PR is waiting to be backported to other branches. label Oct 23, 2018
andrewvc added a commit that referenced this pull request Oct 23, 2018
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 andrewvc deleted the fix-early-conn-close branch October 31, 2018 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Heartbeat using closed connections for body checks with TLS
4 participants