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: indefinite hangs when closing response body #48908

Open
neild opened this issue Oct 11, 2021 · 1 comment
Open

x/net/http2: indefinite hangs when closing response body #48908

neild opened this issue Oct 11, 2021 · 1 comment

Comments

@neild
Copy link
Contributor

@neild neild commented Oct 11, 2021

Closing a response body can block indefinitely under at least two circumstances:

Closing a body with unread data returns flow-control tokens for the unread data. If the underlying net.Conn for the request is write-blocked, the attempt to return the tokens can block either while acquiring the stream write mutex, or while writing the window update to the connection.

Closing a response body waits for the request stream to be cleaned up, or the request context to expire, whichever comes first. This ensures that there is no lingering per-request state remaining after a nil response from resp.Body.Close. (We don't guarantee this property in the package docs, but we do have tests asserting this property. For example, we assume that a request no longer counts against the concurrency limit after a nil response from Close.) If the request writer is blocked reading the request body, we currently have no way to interrupt it and resp.Body.Close will block until the request context is canceled (if ever).

My first thought was that closing the response body could close the request body, interrupting the request writer, but this will require caution: The RoundTripper contract permits RoundTrip to access fields of the request up to the point where the request body is closed. If we change the place we close the request body, we need to be very cautious that we don't access any part of the request after that point.

Test for this second case:

  func TestTransportCloseResponseBodyWhileRequestBodyHangs(t *testing.T) {
          st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {
                  w.WriteHeader(200)
                  io.Copy(io.Discard, r.Body)
          }, optOnlyServer)
          defer st.Close()

          tr := &Transport{TLSClientConfig: tlsConfigInsecure}
          defer tr.CloseIdleConnections()

          pr, pw := net.Pipe()
          req, err := http.NewRequest("GET", st.ts.URL, pr)
          if err != nil {
                  t.Fatal(err)
          }
          res, err := tr.RoundTrip(req)
          if err != nil {
                  t.Fatal(err)
          }
          res.Body.Close()
          pw.Close()
  }
@toothrot toothrot added this to the Backlog milestone Oct 12, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 12, 2021

Change https://golang.org/cl/355491 mentions this issue: http2: close the Request's Body when aborting a stream

gopherbot pushed a commit to golang/net that referenced this issue Oct 13, 2021
After RoundTrip returns, closing the Response's Body should
interrupt any ongoing write of the request body. Close the
Request's Body to unblock the body writer.

Take additional care around the use of a Request after
its Response's Body has been closed. The RoundTripper contract
permits the caller to modify the request after the Response's
body has been closed.

Updates golang/go#48908.

Change-Id: I261e08eb5d70016b49942d72833f46b2ae83962a
Reviewed-on: https://go-review.googlesource.com/c/net/+/355491
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants