net/http: write error from swiftly dropped connection

[rogpeppe]

The following program prints:

got err: <nil>
got body: "the bar is closed\n"
got err: write tcp 127.0.0.1:8080: connection reset by peer

I think the second error is a bug - the http package
isn't waiting for the response before printing the
error from writing to the server.

package main

import (
    "bytes"
    "fmt"
    "io"
    "io/ioutil"
    "log"
    "net/http"
    "strings"
)

func main() {
    http.HandleFunc("/bar", func(w http.ResponseWriter, r *http.Request) {
        http.Error(w, "the bar is closed", 404)
    })
    go http.ListenAndServe(":8080", nil)
    post(strings.NewReader("hello"))
    post(bytes.NewBuffer(make([]byte, 4e6)))
}

func post(r io.Reader) {
    resp, err := http.Post("http://localhost:8080/bar";, "text/utf8", r)
    fmt.Printf("got err: %v\n", err)
    if resp != nil {
        data, err := ioutil.ReadAll(resp.Body)
        if err != nil {
            log.Fatal(err)
        }
        fmt.Printf("got body: %q\n", data)
    }
}

[davecheney]

Comment 1:

I think part of the problem is the call to http.Error causes closeAfterReply to be set
so the connection to the client is severed after the first 404 reply is received.
However it does not appear that the server is indicating this with a 'Connection: close'
header in the first reply body. RFC 2616 8.1.2.1 suggests this is not the correct
behaviour, http://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html#sec8.1.2

[rogpeppe]

Comment 2:

I don't think that's the problem - the client never gets as far as reading the reply
after it gets an error writing the body, so the reply headers shouldn't make any
difference.
For the time being I'm working around this problem by commenting out the
error check after req.Request.write in persistConn.roundTrip.

[bradfitz]

Comment 3:

There'a also a race in this program (the go ListenAndServe may not start before the post
call).
But ignoring that, I'm curious if patching in pending CL
http://golang.org/cl/6213064/ changes anything.

Comment 4:

The server closes the connection before reading the entire request body.  See
http://golang.org/src/pkg/net/http/server.go#L315.
The client fails to write the request and returns an error. This seems correct to me.

[bradfitz]

Comment 5:

Oh, thanks Gary, I missed the "4e6" in there.
Maybe this behavior should be louder somehow.  Print something in the server logs?
Or maybe we should try harder to get our response to the client.  Maybe we instead just
stop reading the large body (but don't close the TCP connection) and instead write our
response + a TCPConn.CloseWrite + actually close the connection after some small-ish
time threshold.

[rogpeppe]

Comment 6:

CL 6213064 doesn't help.
The problem isn't in the server AFAICS. It's in the client - the client gets an error
writing the body and it stops right there. I think it should at least try to read the
response that's waiting to be read.
If there's no response we then have to decide which of the two errors to return. If we
decide that the response-reading error is the one, then the fix is very simple: ignore
any error when writing the body.
BTW here's an actual piece of working code. Sorry about my dodgy copy and paste above.
package main
import (
    "bytes"
    "fmt"
    "io"
    "io/ioutil"
    "log"
    "net"
    "net/http"
    "strings"
)
func main() {
    http.HandleFunc("/bar", func(w http.ResponseWriter, r *http.Request) {
        http.Error(w, "the bar is closed", 404)
    })
    l, err := net.Listen("tcp", ":8080")
    if err != nil {
        log.Fatalf("listen: %v", err)
    }
    go http.Serve(l, nil)
    post(strings.NewReader("hello"))
    post(bytes.NewBuffer(make([]byte, 4e6)))
}
func post(r io.Reader) {
    resp, err := http.Post("http://localhost:8080/bar", "text/utf8", r)
    fmt.Printf("got err: %v\n", err)
    if resp != nil {
        data, err := ioutil.ReadAll(resp.Body)
        if err != nil {
            log.Fatal(err)
        }
        fmt.Printf("got body: %q\n", data)
    }
}

[rogpeppe]

Comment 7:

ah, it seems that it gets corrupted every time.
sigh.

Comment 8:

There is an error in the handler function.  The handler incorrectly writes the response
before reading the request.

[rogpeppe]

Comment 9:

i think it should be fine for the handler to do that.
in this particular case, for instance, i'm emulating an S3 server. the PUT request might
not be allowed (insufficient permissions) but I don't think the server should be obliged
to read the entire body before replying with that error.

[bradfitz]

Comment 10:

So you think the http server should be open to DoS attacks by default?  I don't.
I do think we could do what I said in comment 5, though, not closing the connection,
just doing a TCP write shutdown and a full close after some timeout.  We'll let the
kernel's TCP read buffer fill up and stop acking new packets and hopefully the client
has read our HTTP response by the time we RST on them.

[rogpeppe]

Comment 11:

@bradfitz A timeout seems like a fairly heavy-handed solution to a problem that can be
addressed, I think, with a simple change on the client side. And then it'll work with
any server that does happen to close the connection after responding.

[bradfitz]

Comment 12:

Yes, the client could be fixed too, but there are really two problems here, and in the
Go->Go case, fixing the client first might not actually work:  if the RST comes in too
quickly, we the client might not even be able to read the HTTP response.
Related bug to add 100-continue support to the client is
https://golang.org/issue/3665 but that's in addition to making the
client return a response if it reads one.
For a second I thought that returning err == nil when the Request.Body failed to send
felt wrong, but I realize that's exactly the behavior that 100-continue does anyway, so
I would be fine with it.

Comment 13:

Yeah, the handler should be able to respond without reading the request in this
scenario.  
This simple client fix will work in your configuration, but might not work as expected
in the presence of proxies. The 100-continue support is the reliable way to handle this
scenario.

[bradfitz]

Comment 14:

Here's the client part: http://golang.org/cl/6238043
Note that it doesn't work yet (the test merely logs the failure, but doesn't treat it as
a failure).  The proper fix will require the server not closing so soon I think.  (Feel
free to prove me wrong with code or network traces)
The next thing I'll try to add is having the server send the response and CloseWrite (so
the client can see it), but not do a full Close for 100 ms or so.  (the TCP connection
is considered dead and unavailable for keep-alive both before and after this CL... no
changes there)

[bradfitz]

Comment 15:

Okay, the CL works now.  (The server has been updated too)
Please test/review http://golang.org/cl/6238043 and let me know what you find.
I'm especially curious about how this interacts with other non-Go tools, especially
non-Go clients.
Roger?

Owner changed to @bradfitz.

Status changed to Started.