proposal: x/net/websocket: implement support for websockets over HTTP/2
ethanpailes opened this issue · 4 comments
Overview
I propose extending the x/net/websocket package to support connections made over HTTP/2. This will require no external changes for the server, but the client will need to expose a new mechanism for allowing users to request that HTTP/2 is used to make a connection.
Public Interface Changes
websocket.Config will gain a new HTTP2Transport member of type *http2.Transport
x/net/websocket will gain two new public error types: ErrBadpath and ErrBadHandshake
Having http2 in the name/type of a public member feels a little strange, but storing a generic http.RoundTripper makes it impossible to make sure that we use config.TlsConfig if the user provides one but the transport they provide does not have a TLSClientConfig, and additionally opens up the question of what happens if we are passed a http.Transport rather than a http2.Transport. If someone has even modernly strong opinions about a different color to paint this particular shed, I would love to hear them.
Implementation Notes
A strawman implementation based on the changes I'm proposing in #53208 can be found at https://github.com/ethanpailes/golang-org-x-net/tree/ep/websocket-http2-try3. The main commit is ethanpailes/golang-org-x-net@df38e5d, but I may push little tweaks on top of it.
Enough about the handshake has changed from HTTP/1.1 that the implementation I came up rolls entirely new handshake subroutines for the new transport version. Once the handshake is done, the framing mechanisms are all the same so we can just reuse all the existing infrastructure for that by wrapping out HTTP/2 streams in the right interfaces.
Related Issues
#49918 contains initial discussion about HTTP/2 websocket support
#53208 contains a proposal for adding SETTINGS_ENABLE_CONNECT_PROTOCOL support to x/net/http2, which is required to implement websocket HTTP/2 support.
one limitation on the websocket connection is going to be that it will not be able to set deadlins
var errSetDeadline = errors.New("websocket: cannot set deadline: not using a net.Conn")
I don't really know if this can be a problem, just something to take into consideration
Yeah, that's a good point. I think ideally, the websocket APIs would accept a context to allow support for cancellation, but it's a bit tricky to implement given that HTTP/2 requires sharing a single TCP connection between multiple goroutines. The real fix for this likely involves extending x/net/http2 with a mechanism for setting deadlines for individual reads by adding some machinery so that all reads get routed through a central point that knows how to retry the read if it wasn't supposed to time out. Having a heap of wake times seems like it would work for this.
That's also a pretty big change, so it seems like it would make sense to address it in a separate effort.
any update?