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

Comments

Projects
None yet
5 participants
@bradfitz
Copy link
Member

commented Apr 10, 2016

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

This comment has been minimized.

Copy link

commented Apr 10, 2016

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

gopherbot pushed a commit that referenced this issue Apr 11, 2016

net/http: set the Request context for incoming server requests
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

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2016

(Then I can fix #15927)

@minux

This comment has been minimized.

Copy link
Member

commented Sep 26, 2016

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2016

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

This comment has been minimized.

Copy link

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 label Oct 7, 2016

@gopherbot

This comment has been minimized.

Copy link

commented Oct 15, 2016

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.
You can’t perform that action at this time.