net/http: make http1 Server always in a Read, like http2

[bradfitz]

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.

[gopherbot]

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

[bradfitz]

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)

[ianlancetaylor]

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.

[bradfitz]

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?

[ianlancetaylor]

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.

[ianlancetaylor]

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.

[bradfitz]

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.

[bradfitz]

(Then I can fix #15927)

[minux]

Wow, that's interesting. I didn't know a SetReadDeadline will affect existing blocked Read calls because the docs for SetReadDeadline says: "SetReadDeadline sets the deadline for future Read calls." If this is a capability to be relied upon, should we update the docs for net.Conn? https://golang.org/pkg/net/#Conn

[bradfitz]

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.

[ghost]

@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.

[gopherbot]

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