net/http: CloseNotifier fails to fire when underlying connection is gone

[yichengq]

I observed some odd behavior when using http.CloseNotifier on ReponseWriter in ServeHTTP.

The server program is:

package main

import (
    "log"
    "net/http"
)

func main() {
    http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
        cn, ok := w.(http.CloseNotifier)
        if !ok {
            log.Fatal("don't support CloseNotifier")
        }
        <-cn.CloseNotify()
        log.Printf("CloseNotifier is fired!")
    })
    http.ListenAndServe(":8080", nil)
}

I compiled it using Go 1.5, and ran it on Linux version 3.13.0-48-generic (buildd@orlo) (gcc version 4.8.2 (Ubuntu 4.8.2-19ubuntu1) ) #80-Ubuntu SMP Thu Mar 12 11:16:15 UTC 2015.

It works when I am doing this:

$ ubuntu@ip-172-31-28-238:~$ nc 172.31.28.238 8080
GET / HTTP/1.1

^C

The CloseNotifier is fired as expected.

But it doesn't work when I am doing something like this:

$ ubuntu@ip-172-31-28-238:~$ nc 172.31.28.238 8080
GET / HTTP/1.1

GET / HTTP/1.1

^C

The server keeps a TCP connection at CLOSE_WAIT status, and client leaves a TCP connection at FIN_WAIT2 status. After several minutes, the client TCP connections disappears from Active Internet connections in netstat. After waiting several minutes more, TCP connection at server side becomes can't identify protocol through lsof. That generally means that the underlying socket is CLOSED status but not be closed(https://idea.popcount.org/2012-12-09-lsof-cant-identify-protocol/). In all the time, CloseNotifier is not fired.

The client behavior is simulated from http-pipelining client of python urllib3 described in http://www.projectclearwater.org/adventures-in-debugging-etcd-http-pipelining-and-file-descriptor-leaks/

Any thoughts about this behavior? Is this a bug?

/cc @jonboulle @xiang90 @eyakubovich @bdarnell @philips

from etcd-io/etcd#2468 (comment)

[bdarnell]

This is because http.conn.closeNotify is internally using an unbuffered pipe. Pipelining puts the server in an awkward place: if the client has sent two pipelined requests, the server can no longer detect abandoned connections unless it consumes and buffers the incoming data (up to some reasonable limit). It would be straightforward to add a buffer here but I'm not sure if it's worth it since pipelining is generally (to my knowledge) rare (and should only become more rare, as use cases that benefit from pipelining move to HTTP/2). Even in the case that prompted this investigation, the pipelining was unintentional: it was a result of a connection-reuse bug in the client library. So if it were up to me I probably wouldn't make a change here; it's just one of the several cases in which CloseNotifier cannot promptly detect a closed connection.

[yichengq]

cc bloggers @rkd-msw @bossmc

[vcaputo]

Unless I'm mistaken, it appears the root of this headache is the lack of a mechanism for detecting hangups on the socket out-of-band, so you're forced to read everything until EOF, buffering it in userspace, just to realize the hangup. That's a very significant shortcoming in Golang if accurate.

[bossmc]

A more complex but possibly more efficient solution would be for the HTTP stack to keep pulling requests off the socket as they arrive, and pass each request to a separate goroutine to process (HTTP pipelining is only allowed for idempotent or otherwise orthogonal requests).

The tricky bit here is ensuring that the responses generated by each of those goroutines are written to the wire in the order of their responses (which is probably just extending the implementation of http.ResponseWriter to buffer the local response until the previous writer has written to the wire.

This model will spot all (visible) socket disconnects quickly and also allows the server to parallelize the work for pipelined requests which is the intended benefit of pipelining but I can see your argument that pipelining is being replaced by SPDY/HTTP 2.0 so this might be too involved a change for little gain...

[bradfitz]

This is fixable.

[rsc]

@bradfitz, still think this is fixable?

[bradfitz]

I started on it last week and thought I was going to end up with a net loss of code and less twistiness than today but then things got complicated and I switched to easier boring bugs.

I'll look at it again soon, now that I've thought about it a bit more.

[bradfitz]

@vcaputo,

Unless I'm mistaken, it appears the root of this headache is the lack of a mechanism
for detecting hangups on the socket out-of-band, so you're forced to read everything
until EOF, buffering it in userspace, just to realize the hangup. That's a very
significant shortcoming in Golang if accurate.

Significant shortcoming? What is the portable API for this in any other language?

In any case, I think the CloseNotifier interface and HTTP/1.1 pipelining just aren't compatible. Even if we started separate goroutines for each pipelined request and serialized their responses, any request using CloseNotify with an infinite response would hang all the other requests from replying.

After much discussion with @martine I think the only sane thing to do here is document that CloseNotify fires when either the connection goes to EOF or an HTTP/1.1 pipelined request arrives on the same connection. (HTTP/2 doesn't have this problem). In practice this won't matter because no browser uses pipelining by default because it doesn't work on the Internet. The workaround for people who care is to either use HTTP/2 instead, or only use CloseNotifier with non-idempotent requests (e.g. POST instead of GET) when using HTTP/1.

[bossmc]

Surely any request with an infinite response is an issue, pipelining or not? The HTTP specs are pretty clear about the ordered serialization of pipelined responses so any client that uses pipelining has to be ready to accept that all requests after a slow request will be delayed too and deal with that.

It sounds like your proposal is to not support pipelining and CloseNotifier at the same time, am I understanding correctly? I think this amounts to no longer supporting pipelining since the end of the pipeline is signalled by the client by terminating the connection (which the server now won't be able to detect).

You mention that this won't affect browsers because "it doesn't work on the internet", but it's used extensively by mobile browsers today (http://www.guypo.com/http-pipelining-big-in-mobile/) to cut down on page load times by eliminating high-latency round trips. Also, HTTP is being used more and more for non-internet things (RESTful management APIs, monitoring, message busses) where pipelining is a useful tool for allowing higher throughput of requests in controlled environments (maybe here people should be switching to HTTP/2.0 but not everyone will move over immediately). Removing pipelining support from go feels like a shame.

[bradfitz]

Surely any request with an infinite response is an issue, pipelining or not? The HTTP specs are pretty clear about the ordered serialization of pipelined responses so any client that uses pipelining has to be ready to accept that all requests after a slow request will be delayed too and deal with that.

And we do deal with it. We accept all request as they arrive, but only process them in serial, which fulfills our obligation spec-wise about replying in order.

It sounds like your proposal is to not support pipelining and CloseNotifier at the same time, am I understanding correctly?

Depends what you mean. I'm saying that CloseNotifier would be reworded to guarantee it only works when it actually works today: the last request in a pipeline. So if you wanted to use it reliably, you'd use it on POST requests, or the last GET in a pipeline, or with a GET without any following request. (no pipelining). We would always be in a Read on the client's socket and if they disconnect or send a pipelined request while we have a CloseNotify open, then we send on that CloseNotifier.

Currently we don't document, for instance, that if you're using CloseNotifier on a POST request with a large unread Request.Body, CloseNotifier just won't work.

I think this amounts to no longer supporting pipelining since the end of the pipeline is signalled by the client by terminating the connection (which the server now won't be able to detect).

We detect EOF at the end of request(s) whether they're single, keep-alive serial, or pipelined.

You mention that this won't affect browsers because "it doesn't work on the internet", but it's used extensively by mobile browsers today (http://www.guypo.com/http-pipelining-big-in-mobile/)

You say "today" and then reference an article from 2011. Chrome on Android since then often uses the Google Data Compression (opt-in) which uses SPDY or HTTP/2 for non-HTTPS sides. And HTTP/2 adoption is growing a lot in the last year.

I don't think HTTP/1.1 pipelining ever mattered much and will probably never matter enough.

We support it because we have to, and it's easy enough to support, but I don't think we need to support the combination of two fringe features (HTTP pipelining + CloseNotifier) together especially when there's no common API on all operating systems that Go supports to tell us that a FIN arrived from the client. We do set TCP keep-alives in general and will often fire on that, but again there's no common sideband interface for kernels to tell us that a TCP connection failed its heartbeats.

I'm totally open to suggestions here, but I can't think of anything that works well.

The CloseNotifier interface is just too vague and over-promise-y and it's hard to deliver on it, especially without operating system support. We could poll and parse /proc/net/tcp on Linux but that doesn't scale in several ways.

Removing pipelining support from go feels like a shame.

I'm not proposing removing pipelining support.

In summary, I'm proposing,

1. documenting more explicitly when CloseNotifier is expected to work, restricting people's expectations. I would say "no outstanding data on the connection", which includes pipelined requests. Then I'd call out pipelined requests and say that to guarantee that clients don't send pipelined requests, only use CloseNotifier for POSTs, etc.

2. making sure Go is always in a Read call so we can detect whenever new data (or EOF) arrives on the socket and fire any open CloseNotifier channel.

[bossmc]

Ah, I understand your proposal now, I read

CloseNotify fires when either the connection goes to EOF or an HTTP/1.1 pipelined request arrives on the same connection

as meaning that the connection would be closed if a pipelined request arrived on the connection (hence my deduction that you were removing piplining support), I see now that you just mean that the channel will just send a notification when a piplined request arrives so the handler can react appropriately. That sounds like a good compromise to me.