Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
bradfitz opened this issue Apr 10, 2016 · 12 comments
Closed

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

bradfitz opened this issue Apr 10, 2016 · 12 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@bradfitz
Copy link
Contributor

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.

@bradfitz bradfitz self-assigned this Apr 10, 2016
@bradfitz bradfitz added this to the Go1.7 milestone Apr 10, 2016
@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Apr 11, 2016
Updates #13021
Updates #15224

Change-Id: Ia3cd608bb887fcfd8d81b035fa57bd5eb8edf09b
Reviewed-on: https://go-review.googlesource.com/21810
Reviewed-by: Andrew Gerrand <adg@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@bradfitz
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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 bradfitz modified the milestones: Go1.8, Go1.8Maybe Sep 26, 2016
@bradfitz
Copy link
Contributor Author

(Then I can fix #15927)

@minux
Copy link
Member

minux commented Sep 26, 2016 via email

@bradfitz
Copy link
Contributor Author

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
Copy link

ghost commented Sep 27, 2016

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

@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 7, 2016
@gopherbot
Copy link

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants