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: provide way to get Request write error and HTTP response? #11745

Open
tarm opened this Issue Jul 17, 2015 · 16 comments

Comments

Projects
None yet
10 participants
@tarm

tarm commented Jul 17, 2015

(As Brad mentioned on go-nuts, this is likely a race between two different responses and may not be a bug. Nonetheless, here is a report to track it if you want to consider changing the behavior in the future.)

On linux x64, the following program will go test -v with "Failed 0 out of 100" with go1.4. In go1.5beta1, I often get results like

...
http_test.go:33: 98 Put http://localhost:8080/: write tcp 127.0.0.1:50310->127.0.0.1:8080: write: connection reset by peer
http_test.go:38: Failed 68 out of 100

This seems to be caused by commit 1045351 and reverting that commit makes the test pass again.

package main

import (
        "net/http"
        "strings"
        "testing"
)

const contentLengthLimit = 1024 * 1024 // 1MB                                                                                                                             

func handler(w http.ResponseWriter, r *http.Request) {
        if r.ContentLength >= contentLengthLimit {
                w.WriteHeader(http.StatusBadRequest)
                r.Body.Close()
                return
        }
        w.WriteHeader(http.StatusOK)
}

func TestHandler(t *testing.T) {
        http.HandleFunc("/", handler)
        go http.ListenAndServe(":8080", nil)

        fail := 0
        count := 100
        for i := 0; i < count; i++ {
                r := strings.NewReader(strings.Repeat("a", 10000000))
                req, err := http.NewRequest("PUT", "http://localhost:8080/", r)
                client := &http.Client{}
                resp, err := client.Do(req)
                if err != nil {
                        fail++
                        t.Log(i, err)
                        //t.Fatal(err)                                                                                                                                    
                } else if resp.StatusCode != 400 {
                        t.Fatalf("Expected the status 400, got %v instead.", resp.StatusCode)
                }
        }
        t.Logf("Failed %v out of %v\n", fail, count)
}

@adg adg changed the title from go1.5beta1 http server changes behavior compared to go1.4 to net/http: go1.5beta1 http server changes behavior compared to go1.4 Jul 17, 2015

@mikioh

This comment has been minimized.

Contributor

mikioh commented Jul 17, 2015

@tarm,

See http://golang.org/pkg/net/http/#Client.Do. I think both versions behave identically when you call resp.Body.Close.

@mikioh mikioh changed the title from net/http: go1.5beta1 http server changes behavior compared to go1.4 to net/http: go1.5beta1 http client changes behavior compared to go1.4 Jul 17, 2015

@ianlancetaylor ianlancetaylor added this to the Go1.5Maybe milestone Jul 17, 2015

@tarm

This comment has been minimized.

tarm commented Jul 19, 2015

@mikioh I agree that the client behavior did not change. I think the server changed to call resp.Body.Close() sooner, which changed the result returned by the client in my tests. That may not be a bug.

Specifically, the client documentation makes clear that When err is nil, resp always contains a non-nil resp.Body but it leaves open whether err and resp could both be non-null at the same time. http://golang.org/pkg/io/#Reader is an example were both data and err may be returned from the same call.

In a full duplex connection (like net.Conn or io.ReadWriter), the client can Read() a complete and valid response even as the Write() fails. What should the http.Client return in that case?

Perhaps the title should be "net/http: should http.Client return a non-nil err and resp at the same time?"

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jul 22, 2015

@rsc, I'm wondering if we should do anything to address this.

In summary: the http.RoundTripper interface says you get either a *Response, or an error.

But in the case of a client writing a large request and the server replying prematurely (e.g. 403 Forbidden) and closing the connection without reading the request body, what does the client want? The 403 response, or the error that the body couldn't be copied?

In addressing a server DoS issue, Go 1.5 now more aggressively closes connections instead of reading forever when the Handler doesn't read to EOF.

There are multiple potential fixes, none a complete solution:

a) change the http.RoundTripper interface to say it can return (non-nil, non-nil). This is problematic for compatibility. Old code wouldn't know to potentially close the non-nil *Response.Body when err is already non-nil.

b) make the Server delay the TCP shutdown for N milliseconds after writing the response, thereby increasing the chance that the client sees our response. We do this elsewhere.

c) make the Transport give an N millisecond (optional, tunable on *Transport?) advantage to responses over body write errors. e.g.:

$ git di
diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index 41fc6d0..6d4b50a 100644
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -1165,6 +1165,10 @@ WaitResponse:
                case err := <-writeErrCh:
                        if err != nil {
                                re = responseAndError{nil, err}
+                               select {
+                               case re = <-resc:
+                               case <-time.After(20 * time.Millisecond):
+                               }
                                pc.close()
                                break WaitResponse
                        }

We could do (c) easily and probably even (b) for Go 1.5 if this is a concern. It's very late, though. And we already had this race/ambiguity in all previous Go releases, so maybe it's not a big deal. It's just changed slightly now.

I'd like to think of a better story for Go 1.6 but I don't have any great answers yet.

@nightlyone

This comment has been minimized.

Contributor

nightlyone commented Jul 22, 2015

What about a special error type when implementing a) ?
So a nil response and a type HTTPError struct { Code int; Err error } implementing the error interface can be returned.

Such a type would be extremely useful in a lot of other situations too. So it is very useful to provide it anyway.

@rsc

This comment has been minimized.

Contributor

rsc commented Jul 22, 2015

Doing (c) sounds great for Go 1.5. If (b) is as simple as your patch for (c), we can do (b) too. I agree that doing (a) is basically a non-starter, since it will break all the old code that assumes either-or.

Introducing new API like an HTTPError as in @nightlyone's comment is not something we should do now. It also has serious backwards compatibility issues, since people have learned that HTTP error codes come back as ordinary responses.

@rsc

This comment has been minimized.

Contributor

rsc commented Jul 22, 2015

Brad, is #11020 the same problem?

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jul 23, 2015

No, #11020 is different but easy.

@gopherbot

This comment has been minimized.

gopherbot commented Jul 23, 2015

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

@gopherbot

This comment has been minimized.

gopherbot commented Jul 24, 2015

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

bradfitz added a commit that referenced this issue Jul 27, 2015

net/http: on Transport body write error, wait briefly for a response
From #11745 (comment) :

The http.RoundTripper interface says you get either a *Response, or an
error.

But in the case of a client writing a large request and the server
replying prematurely (e.g. 403 Forbidden) and closing the connection
without reading the request body, what does the client want? The 403
response, or the error that the body couldn't be copied?

This CL implements the aforementioned comment's option c), making the
Transport give an N millisecond advantage to responses over body write
errors.

Updates #11745

Change-Id: I4485a782505d54de6189f6856a7a1f33ce4d5e5e
Reviewed-on: https://go-review.googlesource.com/12590
Reviewed-by: Russ Cox <rsc@golang.org>

bradfitz added a commit that referenced this issue Jul 27, 2015

net/http: pause briefly after closing Server connection when body rem…
…ains

From #11745 (comment)
this implements option (b), having the server pause slightly after
sending the final response on a TCP connection when we're about to close
it when we know there's a request body outstanding. This biases the
client (which might not be Go) to prefer our response header over the
request body write error.

Updates #11745

Change-Id: I07cb0b74519d266c8049d9e0eb23a61304eedbf8
Reviewed-on: https://go-review.googlesource.com/12658
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>

@bradfitz bradfitz modified the milestones: Unplanned, Go1.5Maybe Jul 28, 2015

@bradfitz bradfitz added the Thinking label Jul 28, 2015

@bradfitz bradfitz changed the title from net/http: go1.5beta1 http client changes behavior compared to go1.4 to net/http: provide way to get Request write error and HTTP response? Jul 28, 2015

@tarm

This comment has been minimized.

tarm commented Jan 31, 2017

@bradfitz are you planning further changes for this ticket? If not I think it can be closed.

I opened it because some of our unit tests started failing after the update to 1.5beta1. In addition to the fix c) that you put in, we also modified our unit tests now and this is not an issue for us and likely not for anyone else either.

@bradfitz bradfitz modified the milestones: Go1.9Maybe, Unplanned Jan 31, 2017

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jan 31, 2017

@tarm I'll close or decide what to do during the Go 1.9 cycle.

@d33d33

This comment has been minimized.

d33d33 commented Jul 25, 2017

This issue seems to be back. I am running a program under go1.8.3 which encounter really often the following error: write tcp x.x.x.x:40322->x.x.x.x:443: write: broken pipe. This program perform http request to a server (Jetty) that closed the connection immediately after issuing a 403 error code while the client continue to send data.

The @bradfitz fix seems to have been override by 7fa9846 so I simply reapply the following commit 0c2d3f7. This do the trick, I no longer experience write: broken pipe :)
You can find the full diff here e3adabb

@bradfitz bradfitz removed their assignment Jul 31, 2017

@StevenLeRoux

This comment has been minimized.

StevenLeRoux commented Oct 27, 2017

Hi, I'm facing the same issue.

Currently as a workaround, we maintain our own Golang fork, but it's clearly not a long term solution.

It'd be great to see this fixed for 1.10.

@rsc

This comment has been minimized.

Contributor

rsc commented Nov 22, 2017

Moving to Go 1.11, sorry.

@thinkhy

This comment has been minimized.

thinkhy commented Apr 12, 2018

Any update for this issue? I hit this one when run IPFS.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Apr 12, 2018

Nope. Updates would be posted here.

@gopherbot gopherbot modified the milestones: Go1.11, Unplanned May 23, 2018

jasonwbarnett pushed a commit to jasonwbarnett/fileserver that referenced this issue Jul 11, 2018

net/http: on Transport body write error, wait briefly for a response
From golang/go#11745 (comment) :

The http.RoundTripper interface says you get either a *Response, or an
error.

But in the case of a client writing a large request and the server
replying prematurely (e.g. 403 Forbidden) and closing the connection
without reading the request body, what does the client want? The 403
response, or the error that the body couldn't be copied?

This CL implements the aforementioned comment's option c), making the
Transport give an N millisecond advantage to responses over body write
errors.

Updates #11745

Change-Id: I4485a782505d54de6189f6856a7a1f33ce4d5e5e
Reviewed-on: https://go-review.googlesource.com/12590
Reviewed-by: Russ Cox <rsc@golang.org>

jasonwbarnett pushed a commit to jasonwbarnett/fileserver that referenced this issue Jul 11, 2018

net/http: pause briefly after closing Server connection when body rem…
…ains

From golang/go#11745 (comment)
this implements option (b), having the server pause slightly after
sending the final response on a TCP connection when we're about to close
it when we know there's a request body outstanding. This biases the
client (which might not be Go) to prefer our response header over the
request body write error.

Updates #11745

Change-Id: I07cb0b74519d266c8049d9e0eb23a61304eedbf8
Reviewed-on: https://go-review.googlesource.com/12658
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment