net/http: make http1 Server always in a Read, like http2
bradfitz opened this issue · 12 comments
Changing the net/http Server to always be in a read (like http2) will simplify CloseNotify and make the per-request context cancelation nicely. I'd like the Request's context's Done channel to close when either the ServeHTTP method returns (easy) or when the connection closes.
CL https://golang.org/cl/21810 mentions this issue.
Oh right, I remembered why this is tricky or impossible (I tried to do this previous cycles): we can't proactively be in a read because the next HTTP request might use http.Hijacker and want the unperturbed TCPConn. We can't give them back a different concrete type, and we can't read their bytes for them. (Perhaps we could, because we give back a *bufio.Reader where they should expect to find any over-read bytes, but it would be a big enough behavior change to probably violate the Go 1 API contract in practice)
What I really need in the net/http package is a way to do WaitRead on the *net.TCPConn, waiting to find out when either the connection is readable or closed (anything that would wake up epoll_wait and friends).
Basically I need guts of the net package exposed.
@ianlancetaylor, would you be open to an API addition to the net package, either publicly or via some hacky internal mechanism? (I'd feel a little gross having net/http cheat and do things others couldn't, but not that gross, if we consider it a learning experience before either making it public or removing it in 1.8)
It's not that easy. We use edge-triggered polling. That means that we don't check for whether there is data available. Instead, we arm a trigger, do a non-blocking read, and if that fails, put the goroutine to sleep waiting for the trigger to fire. We don't currently have a mechanism for detecting whether there is data to read without actually reading it.
Looking at net/fd_unix.go,
func (fd *netFD) Read(p []byte) (n int, err error) {
if err := fd.readLock(); err != nil {
return 0, err
}
defer fd.readUnlock()
if err := fd.pd.prepareRead(); err != nil {
return 0, err
}
for {
n, err = syscall.Read(fd.sysfd, p)
if err != nil {
n = 0
if err == syscall.EAGAIN {
if err = fd.pd.waitRead(); err == nil {
continue
}
}
}
err = fd.eofError(n, err)
break
}
if _, ok := err.(syscall.Errno); ok {
err = os.NewSyscallError("read", err)
}
return
}Perhaps we could guarantee the behavior of a net.TCPConn.Read(nil), and lock in the behavior that it goes all the way to a syscall.Read and no fast path does a len(p) == 0 check?
Then the question is what the various kernels do with a zero byte read system call on a TCP socket do. Maybe they return something useful?
On at least some systems the read system call with a length of 0 simply returns 0 without looking at the file descriptor at all.
At least on Unix systems we can find out whether there is data available to read by using SIOCINQ or FIONREAD. But I don't know how we can reliably find out whether the socket has been closed without reading from it.
New plan!
Instead of waiting for readability, I'll just always be in a Read. I forgot that a blocked Read can be interrupted by another goroutine messing with the conn's ReadDeadline. So in the case of a Hijack, I can just SetReadDeadline to zero, wait for the Read to finish, and then proceed with the Hijack, giving the user a net.Conn which won't cause a data race from concurrent Read calls.
IIRC, we have a bunch of tests locking in that behavior, and I think I even fixed some plan9 bugs to make them pass there (or maybe I didn't? @0intro might remember). So maybe we just need to update the docs. I'll file a separate bug for that.
@bradfitz did you file a separate bug for updating the docs? (I can't find one). I'd like to contribute a comment to that change.
CL https://golang.org/cl/31173 mentions this issue.