net/http: Cannot set default 'h2' and other NextProto handler concurrently

[jefferai]

Please answer these questions before submitting your issue. Thanks!

1. What version of Go are you using (go version)?

1.7rc4

1. What operating system and processor architecture are you using (go env)?

(Code only)

1. What did you do?

Took a dive through code trying to work out how to do this.

Here's what I believe the problem to be, although maybe this is purely a documentation bug.

Let's say you want to use the built-in h2 handling but also define your own handler for your own protocol. According to the documentation:

        // TLSNextProto optionally specifies a function to take over
        // ownership of the provided TLS connection when an NPN
        // protocol upgrade has occurred.  The map key is the protocol
        // name negotiated. The Handler argument should be used to
        // handle HTTP requests and will initialize the Request's TLS
        // and RemoteAddr if not already set.  The connection is
        // automatically closed when the function returns.
        // If TLSNextProto is nil, HTTP/2 support is enabled automatically.

So far so good. But what about if TLSNextProto isn't nil? When a Server starts serving (via Serve()), according to the method comments:

// For HTTP/2 support, srv.TLSConfig should be initialized to the
// provided listener's TLS Config before calling Serve. If 
// srv.TLSConfig is non-nil and doesn't include the string "h2" in
// Config.NextProtos, HTTP/2 support is not enabled.

OK, so this appears to indicate that one should specify a NextProtos field in the provided tls.Config including h2, which is fine. But now let's dig into the code.

In function setupHTTP2_Serve() it calls a once object to set up the defaults:

func (srv *Server) setupHTTP2_Serve() error {
        srv.nextProtoOnce.Do(srv.onceSetNextProtoDefaults_Serve)
        return srv.nextProtoErr
}
func (srv *Server) onceSetNextProtoDefaults_Serve() {
        if srv.shouldConfigureHTTP2ForServe() {
                srv.onceSetNextProtoDefaults()
        }
}

Let's look at whether it should configure:

// shouldDoServeHTTP2 reports whether Server.Serve should configure
// automatic HTTP/2. (which sets up the srv.TLSNextProto map)
func (srv *Server) shouldConfigureHTTP2ForServe() bool {
        if srv.TLSConfig == nil {
                // Compatibility with Go 1.6:
                // If there's no TLSConfig, it's possible that the user just
                // didn't set it on the http.Server, but did pass it to
                // tls.NewListener and passed that listener to Serve.
                // So we should configure HTTP/2 (to set up srv.TLSNextProto)
                // in case the listener returns an "h2" *tls.Conn.
                return true
        }
        // The user specified a TLSConfig on their http.Server.
        // In this, case, only configure HTTP/2 if their tls.Config
        // explicitly mentions "h2". Otherwise http2.ConfigureServer
        // would modify the tls.Config to add it, but they probably already
        // passed this tls.Config to tls.NewListener. And if they did,
        // it's too late anyway to fix it. It would only be potentially racy.
        // See Issue 15908.
        return strSliceContains(srv.TLSConfig.NextProtos, http2NextProtoTLS)
}

So if we've put h2 into our tls.Config.NextProtos then we return true, meaning we should be configuring HTTP/2 automatically. So now let's look at the defaults function:

// onceSetNextProtoDefaults configures HTTP/2, if the user hasn't
// configured otherwise. (by setting srv.TLSNextProto non-nil)
// It must only be called via srv.nextProtoOnce (use srv.setupHTTP2_*).
func (srv *Server) onceSetNextProtoDefaults() {
        if strings.Contains(os.Getenv("GODEBUG"), "http2server=0") {
                return
        }
        // Enable HTTP/2 by default if the user hasn't otherwise
        // configured their TLSNextProto map.
        if srv.TLSNextProto == nil {
                srv.nextProtoErr = http2ConfigureServer(srv, nil)
        }
}

The problem is the check for srv.TLSNextProto == nil. The code here doesn't act according to any of the previous documentation; on the user facing side it doesn't explicitly say that HTTP/2 will be activated in this case but doesn't state that this is a case in which it won't be, unlike other cases. The shouldConfigureHTTP2ForServe() check indicates that in fact we should be activating HTTP/2 support because we've explicitly enabled it...but then it isn't.

I'm guessing that the reason behind this is that you don't want to clobber a user-set value for h2 in NextProtos, but since the built-in function isn't exported, there is no way that I can see to specify a custom protocol and have the connection use Go's built-in HTTP/2 handling.

As it stands it seems like my only option as a user is to modify that map after I've called Serve and let the default h2 handler be propagated into it. This will probably work, but seems like a bad idea and potentially racy since once Serve is called reads will be performed on the map.

My suggestion is that the check for srv.TLSNextProto == nil should instead be checking for either nil or checking whether the map already contains a value for h2, and if not, adding the default handling.

[bradfitz]

At most we can document this more, but we're not going to change the behavior here. Go 1.6 had the if srv.TLSNextProto == nil behavior and we're going to stick with it. The current documentation which is misleading you is just not complete enough probably. It was only added recently.

If you want to do something fancy like this, just import "golang.org/x/net/http2" and call http2.ConfigureServer yourself, which will do the right thing and append itself to any existing protocol you have registered.

[jefferai]

Thanks, I'll take a look at that! I figured this would end up as a documentation issue rather than a code change, unfortunately.

Since you know the code well... It appears that modifying the map after I call Serve would do what I want, with the obvious caveat of a race condition on that map. Understanding the risks, from a purely functional standpoint, do you think that would work?

Thanks!

[bradfitz]

Or you could use a dummy net.Listener which just immediately returns EOF and call Serve(dummy) first wit that to force the "h2" registration, then register your own, then call Serve with your real listener(s).

I did something like that in https://go-review.googlesource.com/#/c/25280/3/src/net/http/serve_test.go for a unit test. See https://github.com/golang/go/blob/release-branch.go1.7/src/net/http/serve_test.go#L45-L74

[bradfitz]

type dummyListener struct {}

func (dummyListener) Accept() (net.Conn, error) { return nil, io.EOF }
func (dummyListener) Close() error { return nil }
func (dummyListener) Addr() net.Addr { return net.ParseIP("127.0.0.1") }

....

func main() {
     var srv http.Server
     src.Serve(dummyListener{})  // now h2 is registered
     // now set up other protocols

[jefferai]

Hi @bradfitz,

That's basically exactly what I was asking about. After the first Serve call, to set up the other protos, you'd add them to the NextProtos map...correct? But since the Server is already handling connections, goroutines spun out by the first listener will read the map while you write the new values to it, introducing a race condition. Am I reading that wrong?

[bradfitz]

Serve blocks until the listener is done. Yes, new goroutines are spun up for new connections, but that dummyListener never returns any connection, so things should be quiescent and safe for having its NextProtos mucked with.

Of course, you could also just avoid all those hacks and import golang.org/x/net/http2 instead.

[jefferai]

Aha! That's what I was missing.

I do plan to avoid the hacks; I hadn't seen that function until you pointed me at it, but it looks great. But it's nice to understand the other bit too -- thanks for explaining!

[rsc]

Not sure what the plan is for this, if anything. @bradfitz?

[bradfitz]

@rsc, it's labeled Documentation, so it's lower priority. Those can be done next week after the freeze.

[gopherbot]

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