{"action":"created","issue":{"url":"https://api.github.com/repos/golang/go/issues/16588","repository_url":"https://api.github.com/repos/golang/go","labels_url":"https://api.github.com/repos/golang/go/issues/16588/labels{/name}","comments_url":"https://api.github.com/repos/golang/go/issues/16588/comments","events_url":"https://api.github.com/repos/golang/go/issues/16588/events","html_url":"https://github.com/golang/go/issues/16588","id":169242317,"number":16588,"title":"net/http: Cannot set default 'h2' and other NextProto handler concurrently","user":{"login":"jefferai","id":28627,"avatar_url":"https://avatars.githubusercontent.com/u/28627?v=3","gravatar_id":"","url":"https://api.github.com/users/jefferai","html_url":"https://github.com/jefferai","followers_url":"https://api.github.com/users/jefferai/followers","following_url":"https://api.github.com/users/jefferai/following{/other_user}","gists_url":"https://api.github.com/users/jefferai/gists{/gist_id}","starred_url":"https://api.github.com/users/jefferai/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jefferai/subscriptions","organizations_url":"https://api.github.com/users/jefferai/orgs","repos_url":"https://api.github.com/users/jefferai/repos","events_url":"https://api.github.com/users/jefferai/events{/privacy}","received_events_url":"https://api.github.com/users/jefferai/received_events","type":"User","site_admin":false},"labels":[{"id":150880209,"url":"https://api.github.com/repos/golang/go/labels/Documentation","name":"Documentation","color":"aaffaa","default":false},{"id":373399998,"url":"https://api.github.com/repos/golang/go/labels/NeedsFix","name":"NeedsFix","color":"ededed","default":false}],"state":"open","locked":false,"assignee":{"login":"bradfitz","id":2621,"avatar_url":"https://avatars.githubusercontent.com/u/2621?v=3","gravatar_id":"","url":"https://api.github.com/users/bradfitz","html_url":"https://github.com/bradfitz","followers_url":"https://api.github.com/users/bradfitz/followers","following_url":"https://api.github.com/users/bradfitz/following{/other_user}","gists_url":"https://api.github.com/users/bradfitz/gists{/gist_id}","starred_url":"https://api.github.com/users/bradfitz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bradfitz/subscriptions","organizations_url":"https://api.github.com/users/bradfitz/orgs","repos_url":"https://api.github.com/users/bradfitz/repos","events_url":"https://api.github.com/users/bradfitz/events{/privacy}","received_events_url":"https://api.github.com/users/bradfitz/received_events","type":"User","site_admin":false},"assignees":[{"login":"bradfitz","id":2621,"avatar_url":"https://avatars.githubusercontent.com/u/2621?v=3","gravatar_id":"","url":"https://api.github.com/users/bradfitz","html_url":"https://github.com/bradfitz","followers_url":"https://api.github.com/users/bradfitz/followers","following_url":"https://api.github.com/users/bradfitz/following{/other_user}","gists_url":"https://api.github.com/users/bradfitz/gists{/gist_id}","starred_url":"https://api.github.com/users/bradfitz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bradfitz/subscriptions","organizations_url":"https://api.github.com/users/bradfitz/orgs","repos_url":"https://api.github.com/users/bradfitz/repos","events_url":"https://api.github.com/users/bradfitz/events{/privacy}","received_events_url":"https://api.github.com/users/bradfitz/received_events","type":"User","site_admin":false}],"milestone":{"url":"https://api.github.com/repos/golang/go/milestones/38","html_url":"https://github.com/golang/go/milestones/Go1.8","labels_url":"https://api.github.com/repos/golang/go/milestones/38/labels","id":1709363,"number":38,"title":"Go1.8","description":"","creator":{"login":"griesemer","id":8528975,"avatar_url":"https://avatars.githubusercontent.com/u/8528975?v=3","gravatar_id":"","url":"https://api.github.com/users/griesemer","html_url":"https://github.com/griesemer","followers_url":"https://api.github.com/users/griesemer/followers","following_url":"https://api.github.com/users/griesemer/following{/other_user}","gists_url":"https://api.github.com/users/griesemer/gists{/gist_id}","starred_url":"https://api.github.com/users/griesemer/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/griesemer/subscriptions","organizations_url":"https://api.github.com/users/griesemer/orgs","repos_url":"https://api.github.com/users/griesemer/repos","events_url":"https://api.github.com/users/griesemer/events{/privacy}","received_events_url":"https://api.github.com/users/griesemer/received_events","type":"User","site_admin":false},"open_issues":136,"closed_issues":579,"state":"open","created_at":"2016-04-15T16:30:23Z","updated_at":"2016-11-10T19:15:11Z","due_on":"2017-01-31T00:00:00Z","closed_at":null},"comments":10,"created_at":"2016-08-03T21:47:15Z","updated_at":"2016-11-10T20:30:20Z","closed_at":null,"body":"Please answer these questions before submitting your issue. Thanks!\n1. What version of Go are you using (`go version`)?\n\n1.7rc4\n1. What operating system and processor architecture are you using (`go env`)?\n\n(Code only)\n1. What did you do?\n\nTook a dive through code trying to work out how to do this.\n\nHere's what I believe the problem to be, although maybe this is purely a documentation bug.\n\nLet'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:\n\n```\n        // TLSNextProto optionally specifies a function to take over\n        // ownership of the provided TLS connection when an NPN\n        // protocol upgrade has occurred.  The map key is the protocol\n        // name negotiated. The Handler argument should be used to\n        // handle HTTP requests and will initialize the Request's TLS\n        // and RemoteAddr if not already set.  The connection is\n        // automatically closed when the function returns.\n        // If TLSNextProto is nil, HTTP/2 support is enabled automatically.\n```\n\nSo far so good. But what about if `TLSNextProto` isn't nil? When a `Server` starts serving (via `Serve()`), according to the method comments:\n\n```\n// For HTTP/2 support, srv.TLSConfig should be initialized to the\n// provided listener's TLS Config before calling Serve. If \n// srv.TLSConfig is non-nil and doesn't include the string \"h2\" in\n// Config.NextProtos, HTTP/2 support is not enabled.\n```\n\nOK, 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.\n\nIn function `setupHTTP2_Serve()` it calls a once object to set up the defaults:\n\n``` go\nfunc (srv *Server) setupHTTP2_Serve() error {\n        srv.nextProtoOnce.Do(srv.onceSetNextProtoDefaults_Serve)\n        return srv.nextProtoErr\n}\nfunc (srv *Server) onceSetNextProtoDefaults_Serve() {\n        if srv.shouldConfigureHTTP2ForServe() {\n                srv.onceSetNextProtoDefaults()\n        }\n}\n```\n\nLet's look at whether it should configure:\n\n``` go\n// shouldDoServeHTTP2 reports whether Server.Serve should configure\n// automatic HTTP/2. (which sets up the srv.TLSNextProto map)\nfunc (srv *Server) shouldConfigureHTTP2ForServe() bool {\n        if srv.TLSConfig == nil {\n                // Compatibility with Go 1.6:\n                // If there's no TLSConfig, it's possible that the user just\n                // didn't set it on the http.Server, but did pass it to\n                // tls.NewListener and passed that listener to Serve.\n                // So we should configure HTTP/2 (to set up srv.TLSNextProto)\n                // in case the listener returns an \"h2\" *tls.Conn.\n                return true\n        }\n        // The user specified a TLSConfig on their http.Server.\n        // In this, case, only configure HTTP/2 if their tls.Config\n        // explicitly mentions \"h2\". Otherwise http2.ConfigureServer\n        // would modify the tls.Config to add it, but they probably already\n        // passed this tls.Config to tls.NewListener. And if they did,\n        // it's too late anyway to fix it. It would only be potentially racy.\n        // See Issue 15908.\n        return strSliceContains(srv.TLSConfig.NextProtos, http2NextProtoTLS)\n}\n```\n\nSo 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:\n\n``` go\n// onceSetNextProtoDefaults configures HTTP/2, if the user hasn't\n// configured otherwise. (by setting srv.TLSNextProto non-nil)\n// It must only be called via srv.nextProtoOnce (use srv.setupHTTP2_*).\nfunc (srv *Server) onceSetNextProtoDefaults() {\n        if strings.Contains(os.Getenv(\"GODEBUG\"), \"http2server=0\") {\n                return\n        }\n        // Enable HTTP/2 by default if the user hasn't otherwise\n        // configured their TLSNextProto map.\n        if srv.TLSNextProto == nil {\n                srv.nextProtoErr = http2ConfigureServer(srv, nil)\n        }\n}\n```\n\nThe 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.\n\nI'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.\n\nAs 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.\n\nMy 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.\n"},"comment":{"url":"https://api.github.com/repos/golang/go/issues/comments/259798971","html_url":"https://github.com/golang/go/issues/16588#issuecomment-259798971","issue_url":"https://api.github.com/repos/golang/go/issues/16588","id":259798971,"user":{"login":"gopherbot","id":8566911,"avatar_url":"https://avatars.githubusercontent.com/u/8566911?v=3","gravatar_id":"","url":"https://api.github.com/users/gopherbot","html_url":"https://github.com/gopherbot","followers_url":"https://api.github.com/users/gopherbot/followers","following_url":"https://api.github.com/users/gopherbot/following{/other_user}","gists_url":"https://api.github.com/users/gopherbot/gists{/gist_id}","starred_url":"https://api.github.com/users/gopherbot/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gopherbot/subscriptions","organizations_url":"https://api.github.com/users/gopherbot/orgs","repos_url":"https://api.github.com/users/gopherbot/repos","events_url":"https://api.github.com/users/gopherbot/events{/privacy}","received_events_url":"https://api.github.com/users/gopherbot/received_events","type":"User","site_admin":false},"created_at":"2016-11-10T20:30:20Z","updated_at":"2016-11-10T20:30:20Z","body":"CL https://golang.org/cl/33090 mentions this issue."}}
{"actor":{"display_login":"gopherbot"}}