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

x/net/http2: Transport is leaking streams on broken Body #27208

Closed
euroelessar opened this issue Aug 25, 2018 · 13 comments
Closed

x/net/http2: Transport is leaking streams on broken Body #27208

euroelessar opened this issue Aug 25, 2018 · 13 comments
Assignees
Milestone

Comments

@euroelessar
Copy link

@euroelessar euroelessar commented Aug 25, 2018

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.10.3 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

linux, amd64

What did you do?

  1. Create golang.org/x/net/http2.Transport
  2. Send POST request with non-nil Body
  3. Body returns an error on Read call

What did you expect to see?

Client fails the request and resets the stream.

What did you see instead?

Client fails the request and leaks the stream (on both client and server sides).

@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Aug 25, 2018

Thank you filing this issue @euroelessar and welcome to Go!

So to help with a prognosis, proper fix for this issue and for posterity, it would be helpful to paste in here a runnable repro because the conditions that you are described are part of the usual x/net/http2.Transport usages.

In the meantime I'll page some http2 folks @rs @tombergan @bradfitz

@euroelessar

This comment has been minimized.

Copy link
Author

@euroelessar euroelessar commented Aug 25, 2018

Thanks, @odeke-em.
Pull request has a test which exposes the issue. Though let me try to combine some standalone example as well.

@euroelessar

This comment has been minimized.

Copy link
Author

@euroelessar euroelessar commented Aug 25, 2018

Standalone example: https://gist.github.com/euroelessar/6f4535db11b8bf024d85253cdaa1db1f
It fails against master x/net/http2 package (deadline on the last GET call due to exceeding concurrency limit).

@rs

This comment has been minimized.

Copy link
Contributor

@rs rs commented Aug 25, 2018

In case of request body write error, the stream is not reset/forgotten. The following patch fixes the issue, though I'm not sure a cancel is the right response to such problem spec-wise:

diff --git a/http2/transport.go b/http2/transport.go
index 9d1f2fa..b5c8701 100644
--- a/http2/transport.go
+++ b/http2/transport.go
@@ -1060,7 +1060,12 @@ func (cc *ClientConn) roundTrip(req *http.Request) (res *http.Response, gotErrAf
                        default:
                        }
                        if err != nil {
-                          return nil, cs.getStartedWrite(), err
+                               startedWrite := cs.getStartedWrite()
+                               if startedWrite {
+                                       cc.writeStreamReset(cs.ID, ErrCodeCancel, nil)
+                               }
+                               cc.forgetStreamID(cs.ID)
+                               return nil, startedWrite, err
                        }
                        bodyWritten = true
                        if d := cc.responseHeaderTimeout(); d != 0 {
@FiloSottile FiloSottile added this to the Go1.12 milestone Aug 30, 2018
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 1, 2018

Change https://golang.org/cl/132715 mentions this issue: http2: don't leak streams on broken body

@enocom

This comment has been minimized.

Copy link

@enocom enocom commented Nov 7, 2018

This issue seems to be the same one behind googleapis/google-cloud-go#1211 and hashicorp/vault#5419.

I see this is attached to the Go 1.12 milestone, but the CL hasn't received any feedback. Are there plans to look at this?

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Nov 7, 2018

I plan to look at it. Still targeted towards Go 1.12. (but could also be backported to Go 1.11.x)

@bradfitz bradfitz self-assigned this Nov 7, 2018
gopherbot pushed a commit to golang/net that referenced this issue Nov 7, 2018
Updates golang/go#27208

Change-Id: I5d9a643f33d27d33b24f670c98f5a51aa6000967
GitHub-Last-Rev: 3ac4a57
GitHub-Pull-Request: #18
Reviewed-on: https://go-review.googlesource.com/c/132715
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Nov 7, 2018

Approved & submitted into x/net. Needs bundle into std.

@enocom

This comment has been minimized.

Copy link

@enocom enocom commented Nov 8, 2018

Thanks for the fast response!

@lbernail

This comment has been minimized.

Copy link

@lbernail lbernail commented Nov 8, 2018

Thank you!
It would be great if the fix could be backported to 1.11

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Nov 8, 2018

@gopherbot, please backport to Go 1.11.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 8, 2018

Backport issue(s) opened: #28673 (for 1.11).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 1, 2018

Change https://golang.org/cl/152080 mentions this issue: net/http: update bundled x/net/http2

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.