net/http: add CloseNotifier #2510
Labels
Milestone
Comments
Russ, any objections? I've now seen this request several times in different forms, including from Blake again just now. It'd be an optional interface, like Hijacker. Owner changed to @bradfitz. Status changed to Accepted. |
So, I implemented this, but now I'm not sure how I feel about it. I thought it'd be prettier. http://golang.org/cl/5453063 I think this could all be implemented outside of the http package (by providing an http.Server with a wrapper Listener which returns wrapped Conns which do this magic). I think. But then we could keep the ugliness and complexity out the core. Status changed to Thinking. |
Think of it as a net.Listener than returns net.Conns then, deferring to a tcp listener and tcp conn for most the work, except for bookkeeping which connections are being handled by which handlers. Then you'd need a new Handler wrapper that works with your wrapped connections. As inspiration, see http://golang.org/pkg/http/#TimeoutHandler |
I'm not against that, at all. The problem, well, inconvenience, comes when I can no longer take advantage of all the work the underlying object of http.ResponseWriter was doing because it no longer allows writes after a hijacking. I find myself copy and pasting <ducks> code from Go's src (i.e. WriteHeader). One way around this is to factor out the convenience methods into a public API. |
But a public API for what? You're really not speaking HTTP anymore, and it is unclear that these kinds of helpers for this non-HTTP protocol belong in the HTTP library. As soon as there is a proxy in the middle the semantics of your server break badly. Since the client has to know about this magic anyway, it seems cleaner to hijack the connection and switch to a simpler custom protocol. Russ |
I'm still speaking HTTP. It's knowing when the connection drops that
is important when doing expensive things. I need to cancel those
expensive things if the connection drops.
That doesn't mean I'm no longer speaking HTTP. It's that in a lot of
cases, I don't know what the proper status code is until the expensive
work has completed in some way.
For example:
go func() {
errCh <- somethingLongRunning(closedCh)
}()
select {
case err := <-errCh:
if err != nil {
http.Error(w, err.String(), 500);
}
}
If I have to Hijack, I can no longer use anything convenient in the
http package, even though I'm still speaking HTTP, because they all
take the http.ResponseWriter, or are on it; and that is now rejecting
all calls to any method.
-b
|
I forgot to mention it earlier, but while implementing this, I realized it's very much
related to implementing HTTP/1.1 pipelining. In both cases we need to listen for
writes from the client while the ServeHTTP function is running. Currently we run
ServeHTTP in the same goroutine, which has this comment before it:
// HTTP cannot have multiple simultaneous active requests.[*]
// Until the server replies to this request, it can't read another,
// so we might as well run the handler in this goroutine.
// [*] Not strictly true: HTTP pipelining. We could let them all process
// in parallel even if their responses need to be serialized.
handler.ServeHTTP(w, w.req)
I'd like to also support pipelining at some point (though SPDY is better).
I do agree with Blake, though, that he's not requesting something crazy or non-HTTP-ish.
We do tell people often that the reason you can't kill goroutines is because you're
supposed to send an exit message on a channel to a goroutine instead, telling it to
stop. That's what this bug is about: letting the HTTP server tell a ServeHTTP
goroutine that if it's doing something expensive that it can stop. It's not about
whether it's streaming it or what output format it's sending, or whether the request
will ever end.
|
I didn't know that was a concern. Would something like: func (s *Server) DisconnectChannel(r *Request) (CloseNotifier, error) ... be a less offensive API? My main concern was overhead for the people who don't use this. My current patch above isn't good in that regard, but I think it can go down to ~0 with a small amount of effort. |
A big use case for this is long polling. The fact than IE 9 doesn't support websockets
pretty much means I have to do long polling. Flash is not an option for us.
With long polling I have a select{} on a write channel. The request is hung there until
something comes in on the write channel. I need some way of knowing when an unexpected
disconnect happens. Polling writes is very inefficient.
Hijacking is really wonky. After you Hijack there's no way to Un-Hijack it. I understand
the purpose of the Hijack - start off as HTTP protocol and do something else (websocket
handshake, typically). But hiding the connection object from the public API is just
crude. Right now the only way to make this work is either to modify the core, or Hijack
the connection and duplicate a bunch of code that ResponseWriter is supposed to do.
What it comes down to is I need the connection/buffer objects AND I want to continue
speaking HTTP. There should be no reason I shouldn't be able to do that.
If you really don't want to expose the connection object a function that blocks until
the connection closes OR something is written by ResponseWriter would work. In that case
I can block on my write channel on another goroutine. I don't necessarily need a
callback, I just need to bock the Handler from finishing until a disconnect/write occurs.
|
Looking at the proposed patch, it looks like the code is run on every request whether
you need it or not. How about something like this:
type HandleBlocker interface {
BlockHandler(release chan bool)
}
func (r *response) BlockHandler(release chan bool) {
peeked := make(chan bool)
go func() {
bs, _ := c.buf.Reader.Peek(1)
if len(bs) == 0 {
peeked <- true
}
}()
select {
case <-peeked:
case <-release:
}
}
With that I should be able to do something like this:
func lpHandler(w http.ResponseWriter, req *http.Request) {
//... write chan string, read chan string, etc...
release := make(chan bool)
go func() {
m := <-write
io.WriteString(w, m)
for {
select {
case m := write
io.WriteString(w, m)
default
return
}
}
release <- true
}()
w.(HandleBlocker).BlockHandler(release)
}
What do you think?
|
The code I suggested didn't work as well as I thought. I had an issue where readRequest is called after Peek returns causing readRequest to hang. What I would really like to see is HTTP pipelining instead. A disconnect notification can be sent when the next readRequest fails. It would also be beneficial to break server.go up into more manageable chunks. Perhaps it should be its own package. There also seems to be some slight duplication between server.go and the rest of the net/http library. I started to rewrite it a bit, but my lack of understanding of 100 continue headers makes me a bit nervous. |
Update on this via issue #4403, comment #3: https://golang.org/issue/4403?c=3 |
Update on this via issue #4403, comment #3: https://golang.org/issue/4403?c=3 |
CL out for review at https://golang.org/cl/6867050/ Status changed to Started. |
|
This issue was closed by revision 4fb78c3. Status changed to Fixed. |
This issue was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
by blake.mizerany:
Per email from Brad: It looks like what you don't actually care about owning the TCP connection and ideally just want something like: var clientClosed chan bool cn, ok := w.(http.CloseNotifier) if ok { clientClosed = cn.CloseChannel() // returns a chan bool, written to when/if this http request is closed } select { case <-clientClosed: // cancel expensive stuff case .... } https://groups.google.com/d/topic/golang-nuts/PcEvewBB-AQ/discussionThe text was updated successfully, but these errors were encountered: