websocket.Conn is so close to net.Conn, but not quite

[joonas-fi]

I had a requirement where I just wanted to pipe bytes in both directions over HTTP.

I bridged the gap by making a super small adapter: https://github.com/function61/holepunch-server/blob/master/pkg/wsconnadapter/wsconnadapter.go
With that adapter the WebSocket now implements net.Conn so I can host SSH servers on it etc.

I know we can't make Conn an io.Reader and io.Writer.. well we could but we probably shouldn't, as it would make the API confusing with Write() and Read() operating on bytes, while NextReader() and NextWriter() take messageType..

But, as you saw from my adapter, the difference is so small, but I spent quite some time coding it having to learn more about WebSockets than I wanted, because I had to learn how gorilla/websocket works and why does it work like that (probably because WebSockets don't support streaming a frame with unbounded length, but instead you have to implements streaming by splitting the stream yourself into multiple frames or something like that).

Anyways, the difference is so small, do you guys think we could merge that adapter into module like gorilla/websocket/wsconnadapter, and mention this somewhere in the docs because I don't think my use case is too exotic and I would love more people benefit by not having to go through the same process I did.

Of course the design of my adapter might not be perfect.. perhaps it could be made more composable by writing an adapter where we get a true unbounded io.Writer and io.Reader where we could pipe unbounded amount of data, and those adapters could be used to make net.Conn compatible interfaces and even more.

Sorry if I'm rambling .. I'm a bit tired. :)

See #282

Your code checks for close messages returned form NextReader, but that's not necessary because NextReader returns text and binary messages only. The NextReader doc lists the message types returned and Control Messages doc describes how control messages are handled.

Because your application does not need message framing and it looks like the client will never run in the browser, did you consider hijacking the HTTP connection and using the network connection directly?

[joonas-fi]

I will research your error handling suggestion later (I'm not at home), thanks for the suggestion! I'm not sure if I can hijack the connection, since my app has to be able to traverse through loadbalancers as well. Isn't hijacking escaping the HTTP protocol? F.ex. Cloudflare had to specifically build support for WebSockets to work through their loadbalancing layer, so I think hijacking wouldn't work. But I will think about this as well!

…

On Thu, Oct 25, 2018, 19:18 Steven Scott ***@***.***> wrote: See #282 <#282> Your code checks for close messages returned form NextReader, but that's not necessary because NextReader returns text and binary messages only. The NextReader doc <https://godoc.org/github.com/gorilla/websocket#Conn.NextReader> lists the message types returned and Control Messages doc <https://godoc.org/github.com/gorilla/websocket#hdr-Control_Messages> describes how control messages are handled. Because your application does not need message framing and it looks like the client will never run in the browser, did you consider hijacking <https://godoc.org/net/http#Hijacker> the HTTP connection and using the network connection directly? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#441 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAmdh033Ut12p8YKP53xIfSFyp0UxWF4ks5uoeQ-gaJpZM4X6Vk7> .

The WebSocket protocol "escapes" the HTTP protocol. This package hijacks the connection from the net/http server. That said, it's possible that using the WebSocket protocol will provide greater connectivity.

If your goal is to get through proxy servers, then using CONNECT directly from your client will do just as well as using the WebSocket protocol. This will avoid all the junk in the protocol that your application does not need.

[joonas-fi]

This hijacking suggestion is a great idea for direct connections, but in the repo I linked to I want to support a loadbalancer (or many, even) in front of my project as well. And for every HTTP hop between they have to explicitly implement WebSockets:

• net/http/httputil: add WebSocket support to ReverseProxy golang/go#26937 (Go's reverse proxy does not support this yet)
• https://blog.cloudflare.com/everybody-gets-websockets/ (Cloudflare's announcement for WebSockets support)
• https://github.com/containous/traefik/blob/6dcb51a4bdc0caf14c77320046112af149467970/Gopkg.toml#L104

And these above links imply that even though they support this WebSocket mode of hijacking, no other modes of hijacking (etc. plain TCP) is supported.

With regards to the CONNECT idea, I think its support is really rare in loadbalancers as well:

• Use traefik as proxy for http connections traefik/traefik#634 => Traefik does not support CONNECT
• I didn't find a mention about Cloudflare supporting CONNECT - they probably don't.

I figure CONNECT is really hard to implement securely in loadbalancing middleware, as by definition it descends to lower level protocol (TCP) from a higher level one (HTTP). At least WebSockets can be proxied hop-by-hop. CONNECT loses HTTP semantics on the first hop.

Therefore, my only choice is to use WebSockets (and by extension, my "wsconnadapter") for my project if I am to support loadbalancing. This sentiment seems to be echoed by https://github.com/jpillora/chisel (does similar port-forwarding things as my holepunch-server project)

And for the record, I have a legitimate use case for this - I will schedule the holepunch-server on my cluster with managed replication, so I don't know which actual IP it will be hosted at - it can even change due to re-scheduling / node outages. Therefore I have Traefik sitting as a frontend in my cluster and point the DNS A record at Traefik, which will automatically forward the HTTP request to the correct container based on hostname.

Thanks for the great feedback on my code - I will add a mutex and investigate the error handling suggestion!

It's impossible to implement the net.Conn deadline semantics with this package. A timeout is not fatal in a net.Conn, but it is with a websocket connection. The net.Conn deadline methods can be called concurrently with read or write and possibly cancel an outstanding operation. Again, that's not something possible with a websocket connection. Maybe it should be, but it's not a simple code change to implement.

These limitations may may not be an issue within joonas-fi's application.

I think it's a mistake to include something in this project that purports to be net.Conn, but is not in subtle and not so subtle ways.

How about including an io.Reader and io.Writer wrappers around a websocket connection? That encapsulates most of the logic needed for these sorts of things.

Proposed reader API:

// JoinMessages concatenates received messages to create a single io.Reader.
// The separator string sep is placed between received messages. The returned
// reader does not support concurrent calls to the Read method.
func JoinMessages(c *Conn, sep string) io.Reader {
}

This function handle the case where message boundaries have some meaning and the case where the peer is sending a stream of bytes.

There are two options for how reads to the network are handled:

Option A:

 // Each call to Read reads at most one message from connection.
 // If the peer sends an empty message, then Read can return with
// no data.

Option B:

// Read consumes messages until data can be returned to the 
// application.

Option A makes it useful to set deadlines between calls to Read, but might cause some confusion because Read can return with no data. Option B will be less surprising to applications.

Either way, applications can set deadlines in a pong handler.

With this proposed API, this chunk of code in the command example can be replaced by:

 io.Copy(w, websocket.JoinMessages(ws, "\n"))

I have second thoughts about providing a function for io.Writer. The io.Writer is simple to implement. This package does not need to provide a helper for every simple thing an application might want to do.

As shown by OPs code, the io.Reader is tricky to get right. There's value in providing that code.

[joonas-fi]

@garyburd, thanks it was a great catch, noticing that I need a sync.Mutex!

Also thanks @stevenscott89 for pointing out that NextReader() does not return CloseMessages.

I have updated my code accordingly.

I like the idea of websocket.JoinMessages(ws, "") for my use case. I also like the idea of not providing net.Conn but io.Reader and io.Writer (although you said you don't desire to implement the this because it's too simple) separately. This is better for composability - I can then compose my net.Conn from the io.Reader and the io.Writer - an idea that I half-suggested in my initial message.

Even if you decide not to implement the corresponding io.Writer adapter, I think JoinMessagesReader() would be a better, more explicit, name for the API.

One additional idea, and I'm just spitballing here, is to implement gorilla/websocket/wsutils if you don't want to pollute the main module ("core") with miscellaneous helpers like this JoinMessagesReader().

As for @stevenscott89's options A and B, if we look at the io.Reader interface https://golang.org/src/io/io.go#L49 a few things stand out:

If some data is available but not len(p) bytes, Read conventionally returns what is available instead of waiting for more.

and

Implementations of Read are discouraged from returning a zero byte count with a nil error, except when len(p) == 0.

however I feel that it contradicts a bit with the very next phrase:

Callers should treat a return of 0 and nil as indicating that nothing happened; in particular it does not indicate EOF.

I feel that the io.Reader interface spec is a bit ambiguous on this, but if having to decide something I feel that option B is more in line with the spec, and as @stevenscott89 said, probably less surprising to applications. Personally, I think option A would have been technically cleaner.