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

[feature] expose newConn function to do websocket protocol on arbitrary net.Conn #573

Closed
djherbis opened this issue Feb 6, 2020 · 13 comments

Comments

@djherbis
Copy link

djherbis commented Feb 6, 2020

I'd like to be able to call newConn to create a websocket endpoint from a net.Conn. This is useful when I already have a net.Conn which I want to do the websocket protocol on.

The solution could be as simple as making this function public. Though a nicer function interface could be provided, like NewConn(net.Conn, options) or something.

My current alternative is to link this function into my own packages when needed, but would def. be safer to use it as part of the public interface.

@ghost
Copy link

ghost commented Feb 6, 2020

func dialWithConn(c net.Conn) (*websocket.Conn, *http.Response, error) {
	d := websocket.Dialer{
		NetDial: func(network, addr string) (net.Conn, error) { return c, nil },
	}
	return d.Dial("http://example.com/", nil)
}


@djherbis
Copy link
Author

djherbis commented Feb 6, 2020

I need to wrap both sides (server and client).

That trick is cool for getting a client-side conn, but I don't see an equiv. solution for a server-side one.

@djherbis
Copy link
Author

djherbis commented Feb 6, 2020

I suppose server-side might be possible if I hand-craft a ResponseWriter with Hijack() set to my net.Conn and configured the Request to look like a websocket request.

Though this all feels pretty hacky just to be able to get a websocket.Conn on an arbitrary net.Conn (server/client).

@ghost
Copy link

ghost commented Feb 6, 2020

Yup, make a ResponseWriter with Hijack.

A little hack in your code is better than extending the package API to support a single user of the feature. The maintainers of this package (not me) may have a different opinion.

Edit: fix typo

@djherbis
Copy link
Author

djherbis commented Feb 6, 2020

The hacks might be more painful than originally thought, since there is some interaction on the net.Conn before it's handed back as a websocket.Conn (in both Dial/Upgrade cases). I'm worried about this being quite brittle for production code, which is the only reason I'd like to avoid my current hack of directly linking to the private method.

I may be one 'user of the package' but my services serve many users. I feel like this would be a helpful addition for my use case, but happy to discuss with other users/maintainers.

@IngCr3at1on
Copy link

I'm missing something I think; the only time newConn is called for a server appears to be during Upgrade and it's not until after it uses the ResponseWriter provided to Upgrade to Hijack the connection. What exactly is wrong with implementing a custom Hijack for this as @srybacki suggested (this doesn't seem "hacky" or "brittle" to me but rather the correct way to use your own net.Conn)?

To be clear, yes Upgrade does interact with the websocket.Conn before handing it back but all it does is set timeout values which should always be set.

I have to wonder though; why would you want to provide your own net.Conn in the first place? It's not like you can use it for something else after the fact; if anything it's wanting to do this in the first place that seems brittle to me vs allowing the websocket package to acquire this for you as it's intended.

@djherbis
Copy link
Author

@IngCr3at1on I use Go's reverse proxy to serve some websocket connections from a downstream server, it doesn't actually use the gorilla package to serve the websocket since this is built into the std lib of Go.

I hijack the proxying net.Conn and wrap it in my own 'teeing' net.Conn, and use gorilla's newConn => *websocket.Conn on that so that I can read the tee'd websocket messages. I use newConn with both directions (client/server) so that I can parse the tee'd messages being sent/recv on the proxy server.

Websocket dialing and upgrading does do more than just set timeouts to the underling net.Conn, they also do some protocol read/write to the underlying net.Conn, which is already done by the time I get access to the net.Conn from the proxy. newConn is also used in a test for this package on a 'fakeConn' impl.

I'm confident I probably can write a bunch of wrappers that just ignore all the stuff happening in Dial/Upgrade to the net.Conn to make this work, however I mainly created this bug to see if this project would even consider providing the raw method so that we could do the message protocol more generally on any old byte-pipe to address more use cases (like mine). Maybe you want to do the websocket protocol on something that isn't an HTTP connection (ex. maybe you have some raw binary websocket message data in a server log file you want to read).

One of Go's strengths is its interfaces for things like net.Conn. It's awesome to be able to wrap/modify a net.Conn's behavior when you want to, or write your own new one. They even provide net.Pipe() which is a fun in-memory net.Conn that comes in handy for testing and even some real scenarios.

Either way, I'm not criticizing a decision not to expose this method, just providing my context on how I would I appreciate it being available.

@djherbis
Copy link
Author

I managed to get it to work without newConn, though it takes ~100 LOC instead of 1 function call, that's still fine.

Because I need to skip the handshake I ended up setting up a tiny http.Server, feeding it one end of a net.Pipe() and the other end to a custom websocker.Dialer. I let both sides finish the handshake and stop the server, then, because I wrapped the net.Conn they were working on, I just swapped out the net.Conn with the tee'd net.Conn I wanted to do the message protocol on and I'm done.

@ghost
Copy link

ghost commented Mar 13, 2020

Do you need a fully functioning connection that handles PINGs, CLOSE and what not, or is your goal to parse messages from an io.Reader? I don't have a good suggestion for the latter, but I think it will be helpful to record the actual requirements should this come up again.

@djherbis
Copy link
Author

Technically I need to parse anything on the wire of a real connection, after the handshake. s
So that would include PINGs, CLOSE etc. The PINGs themselves aren't really doing anything for me, but they will be on the wire I'm reading so they need to be handled, CLOSE is interesting to know when/why the websocket ended.

Technically I'm also only using the read-end of the websocket, not writing to it. I just read the websocket messages that the server would read, and I also read the messages that the server would write.

@ghost
Copy link

ghost commented Mar 13, 2020

I get that you want to read all messages.

Connections respond to PING by sending a PONG. Connections handle CLOSE by echoing the CLOSE back to the peer. It sounds like you don't need this.

@djherbis
Copy link
Author

Correct, the net.Conn impl I use just drops all Writes including (PONG, echo'd CLOSE).

@adhocore
Copy link

i was running into a situation where it required newConn() to be exposed. i needed to have a custom Upgrader.

is it possible to have it as NewConn() or if not what would be the workaround?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants