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

net/http: support concurrent Request.Body reads & ResponseWriter.Write calls in HTTP/1.x server #15527

Open
exel1mailru opened this Issue May 3, 2016 · 15 comments

Comments

Projects
None yet
7 participants
@exel1mailru

exel1mailru commented May 3, 2016

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
    Go 1.5.2
  2. What operating system and processor architecture are you using (go env)?
    windows_amd64
  3. What did you do?
    If possible, provide a recipe for reproducing the error.
    A complete runnable program is good.
    A link on play.golang.org is best.

https://play.golang.org/p/DaWZXCQNfV
this example ends up with error "http: invalid Read on closed Body"

https://play.golang.org/p/WYCsIQzx_F
but changing 100 to 10 in strings.Repeat goes without any errors

also this example https://play.golang.org/p/YxjKnmgfGP
$ curl -d 'qweqweq weq weqwe qwe qew qwe' http://localhost:6060/
<qweqweq weq weq><we qwe qew qwe >

shows that reading and writing goes simultaneously and without errors

  1. What did you expect to see?
    i expect to see the same error in both cases
  2. What did you see instead?
    i see different behaviour

@bradfitz bradfitz changed the title from Writing to http.ResponseWriter closes request.Body, but seems not always to net/http: writing to http.ResponseWriter closes request.Body, but seems not always May 4, 2016

@bradfitz bradfitz added this to the Go1.7 milestone May 4, 2016

@bradfitz bradfitz self-assigned this May 4, 2016

@bradfitz bradfitz modified the milestones: Go1.8Maybe, Go1.7 May 10, 2016

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 10, 2016

I've sent https://golang.org/cl/23011 to doucment the status quo.

We can keep this bug open to track making the HTTP/1.x server support concurrent reads & writes like HTTP/2.

@bradfitz bradfitz changed the title from net/http: writing to http.ResponseWriter closes request.Body, but seems not always to net/http: support concurrent Request.Body reads & ResponseWriter.Write calls in HTTP/1.x server May 10, 2016

@gopherbot

This comment has been minimized.

gopherbot commented May 11, 2016

CL https://golang.org/cl/23011 mentions this issue.

gopherbot pushed a commit that referenced this issue May 11, 2016

net/http: document ResponseWriter read-vs-write concurrency rules
Summary: Go's HTTP/1.x server closes the request body once writes are
flushed. Go's HTTP/2 server supports concurrent read & write.

Added a TODO to make the HTTP/1.x server also support concurrent
read+write. But for now, document it.

Updates #15527

Change-Id: I81f7354923d37bfc1632629679c75c06a62bb584
Reviewed-on: https://go-review.googlesource.com/23011
Reviewed-by: Andrew Gerrand <adg@golang.org>
@bradfitz

This comment has been minimized.

Member

bradfitz commented May 11, 2016

@dpiddy points out the closest thing in RFC 2616 about this topic:

        If an origin server receives a request that does not include an
        Expect request-header field with the "100-continue" expectation,
        the request includes a request body, and the server responds
        with a final status code before reading the entire request body
        from the transport connection, then the server SHOULD NOT close
        the transport connection until it has read the entire request,
        or until the client closes the connection. Otherwise, the client
        might not reliably receive the response message. However, this
        requirement is not be construed as preventing a server from
        defending itself against denial-of-service attacks, or from
        badly broken client implementations.
@herrberk

This comment has been minimized.

herrberk commented May 4, 2018

I believe this is a critical issue affecting many users out there and should be addressed as quickly as possible. I was able to reproduce this issue using Transfer-Encoding: chunked as I explained in this related issue:

This sample server-client pair reproduces this issue:
https://play.golang.org/p/G8F0X4DQCDn

Please let me know if any assistance is needed to fix this issue for the next release. Thanks!

@Stebalien

This comment has been minimized.

Stebalien commented Jun 28, 2018

Relevant HTTP WG email thread: https://lists.w3.org/Archives/Public/ietf-http-wg/2004JanMar/0041.html

TL;DR: Yes, you can respond before reading the entire request.


        If an origin server receives a request that does not include an
        Expect request-header field with the "100-continue" expectation,
        the request includes a request body, and the server responds
        with a final status code before reading the entire request body
        from the transport connection, then the server SHOULD NOT close
        the transport connection until it has read the entire request,
        or until the client closes the connection. Otherwise, the client
        might not reliably receive the response message. However, this
        requirement is not be construed as preventing a server from
        defending itself against denial-of-service attacks, or from
        badly broken client implementations.

This section should be interpreted exactly as written. The server can't close the connection before reading the entire body of the request for the reason stated in that section. That doesn't mean the server can't respond concurrently.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jun 28, 2018

@Stebalien, thanks for finding that.

FWIW, Go's HTTP/1.x server originally permitted this but we encountered enough confused implementations in the wild that would end up deadlocking if they got a response before they were expecting it. (e.g. somebody sent us a POST + body but the Go server sent an Unauthorized response before reading the body). The peer would then deadlock, never reading the response because it wasn't finished writing its body, and we weren't reading their body because our Handler was done running. We've changed behavior a few times over the years (read to EOF, read some, close immediately) and I can't even remember what we do now.

Still worth revisiting, but I suspect we might need to make this behavior opt-in for HTTP/1.x on a per-Handler basis somehow.

@herrberk

This comment has been minimized.

herrberk commented Jun 28, 2018

@Stebalien @bradfitz I understand, permitting this can cause issues like you mentioned but it's already there for HTTP/2, so IMHO it should be added to HTTP/1.x at least as an opt-in feature.

@bradfitz bradfitz removed this from the Go1.11 milestone Jun 28, 2018

@bradfitz bradfitz added this to the Go1.12 milestone Jun 28, 2018

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jun 28, 2018

@herrberk, that's why this bug is open. But it's too late for Go 1.11.

@herrberk

This comment has been minimized.

herrberk commented Jun 28, 2018

@bradfitz okay, no problem as long as it gets resolved eventually. Thanks! :)

@Stebalien

This comment has been minimized.

Stebalien commented Jun 29, 2018

The peer would then deadlock, never reading the response because it wasn't finished writing its body, and we weren't reading their body because our Handler was done running.

Those clients are expecting the server to finish reading the body at some point, as per the spec. The correct solution would be to call Close on req.Body after Handler.ServeHTTP returns, draining the body in the process.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jun 29, 2018

The correct solution would be to call Close on req.Body after Handler.ServeHTTP returns, draining the body in the process.

We've tried variants of that in the past. If you consume the body without bound, you have a DoS vector, letting servers read unbounded amounts of client data. If you specify some magic threshold point where it's acceptable for the server to read-to-discard, then you have confused user bug reports about why things only work sometimes for some sizes.

@herrberk

This comment has been minimized.

herrberk commented Jun 29, 2018

We've tried variants of that in the past. If you consume the body without bound, you have a DoS vector, letting servers read unbounded amounts of client data. If you specify some magic threshold point where it's acceptable for the server to read-to-discard, then you have confused user bug reports about why things only work sometimes for some sizes.

The same must be valid for HTTP/2 as well then, how does HTTP/2 solve that issue of unbounded amount of client data?

@Stebalien

This comment has been minimized.

Stebalien commented Jun 29, 2018

If you specify some magic threshold point where it's acceptable for the server to read-to-discard, then you have confused user bug reports about why things only work sometimes for some sizes.

That's actually what net/http does right now; it reads at most 256KiB for this exact reason. Unfortunately, it does this before sending the reply instead of after the request handler exits.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jun 29, 2018

That's actually what net/http does right now; it reads at most 256KiB for this exact reason.
Unfortunately, it does this before sending the reply instead of after the request handler exits.

Yeah, the current situation is not ideal. It's not clear what ideal even is, which is part of the reason this bug is still open. It needs some thought more than it needs just some code.

People often like to propose changes considering their one use case or failure mode without considering the dozen related ones that have come before theirs which might interfere, which requires poring over the code, comments, git history, bug tracker, and the mailing list. Which is why I'm reluctant to change anything too quickly at this point without careful review.

@Stebalien

This comment has been minimized.

Stebalien commented Jun 29, 2018

People often like to propose changes considering their one use case or failure mode without considering the dozen related ones that have come before theirs which might interfere, which requires poring over the code, comments, git history, bug tracker, and the mailing list. Which is why I'm reluctant to change anything too quickly at this point without careful review.

I understand this is tricky. However, looking through the code and some of the related conversations, it's clear that nobody has all the context (not unusual for complex code like this). Incorrect assumptions about both the current implementation and the spec seem to be quite prevalent so I'm trying to correct those assumptions.

While I'm not asking for an immediate change, simply letting this bug age isn't going to get us anywhere either.

@keks keks referenced this issue Oct 4, 2018

Open

New RPC system #37

@bradfitz bradfitz modified the milestones: Go1.12, Unplanned Nov 1, 2018

@bradfitz bradfitz removed their assignment Nov 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment