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

[exel1mailru]

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]

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.

[gopherbot]

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

[bradfitz]

@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]

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]

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]

@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]

@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]

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

[herrberk]

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

[Stebalien]

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]

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]

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]

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]

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]

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.

[Anonymous-Coward]

@Stebalien @bradfitz Got here via https://github.com/caddyserver/caddy/issues/3557. I believe this bug is related to the one I linked to. I can't help with the bug - could never bring myself to invest enough time to learn go properly. But I'll try to add some perspective on it.

We run several distinct proxies/embedded web servers in our system - envoy, traefik, tomcat (different versions), netty, node's built-in server etc. in addition to caddy. Caddy is the only Go-based one - and the only one exhibiting the problem of prematurely closing connections to clients when the full response is sent out before the full request is received. We also run various clients, and none gets confused if it receives the full response before finishing sending the request.

We provide some APIs, of which we don't control all. Some occasionally send huge requests and responses. Having to fully buffer requests and responses all the time adds significant latency, when those are large.

The definite case where you absolutely want concurrent response and request, in a proxy or web server, is 503. If you limit the number of simultaneous in-flight requests, in order to prevent a server from crashing from memory exhaustion, and respond 503 whenever the server reaches the limit of requests it is allowed to process concurrently, forcing it to still receive the full request before responding defeats the purpose of 503. You keep the server busy when all it wants to do is to tell it's already busy enough already, and won't process your request. The added delay also prevents load balancers from retrying early. This makes workarounds damaging in themselves - if a Go-based proxy is in-between, there's no way around fully buffering the request and response before sending them through the Go proxy.