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

NetConn adapter #100

Closed
nhooyr opened this issue Jun 24, 2019 · 17 comments · Fixed by #102
Closed

NetConn adapter #100

nhooyr opened this issue Jun 24, 2019 · 17 comments · Fixed by #102
Milestone

Comments

@nhooyr
Copy link
Contributor

nhooyr commented Jun 24, 2019

Was going through some random reading and I noticed this is an issue that appeared multiple times on the gorilla/websocket tracker:

People using nhooyr/websocket also have ran into this: #80

With my solution in #87, this is relatively simple to implement.

I'm thinking websocket.NetConn(c *websocket.Conn) net.Conn

@nhooyr
Copy link
Contributor Author

nhooyr commented Jun 24, 2019

Maybe not net.Conn but at least io.ReadWriteCloser as SetDeadline won't work as expected as described in #80.

But at that point, it seems not nearly as useful. 🤔

@nhooyr
Copy link
Contributor Author

nhooyr commented Jun 28, 2019

I think net.Conn is fine with the documented limitation. It's unfortunate but will work fine for 99% of use cases.

@jancona
Copy link

jancona commented Jun 30, 2019

I just ran into this while trying to use net/rpc. rpc.NewClient() and rpc.ServeConn() both take io.ReadWriteCloser. So even that rather than a full net.Conn would work for me.

@nhooyr
Copy link
Contributor Author

nhooyr commented Jul 1, 2019

sg.

Out of curiosity, why use net/rpc with websockets?

nhooyr added a commit that referenced this issue Jul 1, 2019
@nhooyr nhooyr added this to the v1.3.0 milestone Jul 1, 2019
@nhooyr
Copy link
Contributor Author

nhooyr commented Jul 1, 2019

@jancona could you test the latest master to ensure it works properly with your code?

@nhooyr nhooyr reopened this Jul 1, 2019
@jancona
Copy link

jancona commented Jul 3, 2019

It seems like every Read() by the client terminates with an EOF. Is that expected behavior? (I'm not a websockets expert.) In any case, that causes problems for net/rpc which seems to be expecting a stream.

You asked earlier about my use case for net/rpc. I have a Go server and a GopherJS client running in the browser. I want to use net/rpc to allow the server to call a method to display data in the browser. It works using golang.org/x/net/websocket on the server and github.com/gopherjs/websocket on the client. Since golang.org/x/net/websocket is deprecated, I was looking to switch to this package. I know it's a weird use case and I don't blame you if you don't see much value in supporting it! Let me know if you want to pursue this. If so, I can put together a minimal test case.

@nhooyr
Copy link
Contributor Author

nhooyr commented Jul 3, 2019

It seems like every Read() by the client terminates with an EOF. Is that expected behavior? (I'm not a websockets expert.) In any case, that causes problems for net/rpc which seems to be expecting a stream.

Definitely not. It's a bug, will fix, thank you for testing.

You asked earlier about my use case for net/rpc. I have a Go server and a GopherJS client running in the browser. I want to use net/rpc to allow the server to call a method to display data in the browser. It works using golang.org/x/net/websocket on the server and github.com/gopherjs/websocket on the client. Since golang.org/x/net/websocket is deprecated, I was looking to switch to this package. I know it's a weird use case and I don't blame you if you don't see much value in supporting it! Let me know if you want to pursue this. If so, I can put together a minimal test case.

Seems reasonable to me.

nhooyr added a commit that referenced this issue Jul 3, 2019
@nhooyr
Copy link
Contributor Author

nhooyr commented Jul 3, 2019

Thanks again for testing. I believe I fixed the bug + added a test, please try again @jancona.

@jancona
Copy link

jancona commented Jul 3, 2019

That fixes it--thanks! Will you be tagging a new release?

@nhooyr
Copy link
Contributor Author

nhooyr commented Jul 3, 2019

Yup, tagging it now.

@nhooyr nhooyr closed this as completed Jul 3, 2019
@awfulcooking
Copy link

Few users of the library will need this but it's tricky to implement correctly and so provided in the library.

Thanks for implementing this, @nhooyr, it's really handy & appreciated here :)

@maggie44
Copy link
Contributor

maggie44 commented Oct 30, 2023

I'm stumped on an issue using the NetConn adapter.

I have the below for communicating via a websocket to a remote docker instance using Docker's Go SDK (client is the Docker SDK package):

	dockerHost := "http://docker"
	wrappedConn := websocket.NetConn(ctx, c, websocket.MessageBinary)

	// Custom dial function that returns the wrapped WebSocket connection
	customDial := func(ctx context.Context, network, addr string) (net.Conn, error) {
		return wrappedConn, nil
	}
	cli, _ := client.NewClientWithOpts(client.WithDialContext(customDial), client.WithHost(dockerHost))

On the receiving end I accept the request and relay to the Docker socket:

...
	// Connect to Docker Unix Socket with context
	dialer := &net.Dialer{}
	dockerConn, err := dialer.DialContext(ctx, "unix", "/var/run/docker.sock")
	if err != nil {
		slog.Error("error connecting to Docker daemon", "error", err)
		return
	}
	defer dockerConn.Close()

	// Add a long deadline timeout as a fallback for blocked connections
	dockerConn.SetDeadline(time.Now().Add(6 * time.Hour))

	// Relay from Docker to WebSocket
	var wg sync.WaitGroup
	wg.Add(1)
	go func() {
		defer wg.Done()
		_, err := io.Copy(websocket.NetConn(ctx, wsConn, websocket.MessageBinary), dockerConn)
		if err != nil && ctx.Err() == nil {
			slog.Error("error relaying data from Docker socket to WebSocket", "error", err)
		}
	}()

	// Relay from WebSocket to Docker
	_, err = io.Copy(dockerConn, websocket.NetConn(ctx, wsConn, websocket.MessageBinary))
	if err != nil {
		slog.Error("error relaying data from WebSocket to Docker socket", "error", err)
	}

It works great out the box, brilliant feature, with one exception. When trying to connect to attach to a container, the Docker API hijacks the connection and for some reason this seems to stump the NetConn:

	execID, err := cli.ContainerExecCreate(ctx, containerID, execConfig)
	if err != nil {
		panic(err)
	}

	// Attach to the exec instance
	resp, err := cli.ContainerExecAttach(ctx, execID.ID, types.ExecStartCheck{})
	if err != nil {
		panic(err)
	}
	defer resp.Close()

Error message:

2023/10/30 17:45:19 Unsolicited response received on idle HTTP channel starting with "HTTP/1.1 101 UPGRADED\r\nApi-Version: 1.43\r\nConnection"; err=<nil>

The error varies on each request

2023/10/30 17:52:22 Unsolicited response received on idle HTTP channel starting with "HTTP/1.1 101 UPGRADED\r\nApi-Version: 1.43\r\nConnection: Upgrade\r\nContent-Type: application/vnd.docker.multiplexed-stream\r\nDocker-Experimental: false\r\nOstype: linux\r\nServer: Docker/24.0.6 (linux)\r\nUpgrade: tcp\r\n\r\n"; err=<nil>
2023/10/30 17:53:37 Unsolicited response received on idle HTTP channel starting with "HTTP/1.1 101 UPGRADED\r\nApi-Version: 1.43\r\n"; err=<nil>

I'm stumped on this one, any ideas would be welcome. The error message are coming from the Docker client, not the websockets, but for some reason the websocket is modifying the response. When connecting directly to the UNIX socket implementation it works ok.

@maggie44
Copy link
Contributor

maggie44 commented Oct 30, 2023

It seems like it does work ok, but only if it is the first request. If I call cli.ContainerExecAttach it goes through, but if I call cli.ContainerExecCreate and then cli.ContainerExecAttach on the same connection consecutively it errors. Something about cli.ContainerExecAttach specifically which can do a hijack when it's called first but not if something has been called before it. Other non-hijack consecutive commands go through ok. Is there something written on the websocket between each read/write?

I am exploring Docker side as well of course, very possible this has nothing to do with the websocket.

EDIT:

Managed to narrow it down to: https://github.com/moby/moby/blob/311b9ff0aa93aa55880e1e5f8871c4fb69583426/client/hijack.go#L86C1-L86C1

Hard to understand why it would conflict with the websocket connection only on second requests.

@nhooyr
Copy link
Contributor Author

nhooyr commented Oct 30, 2023

Hmm not sure, but can you open a new issue @maggie44

@anderspitman
Copy link

@nhooyr thanks for making this wrapper, it's working excellently for me so far.

I'm trying to make sure I understand the caveats. It should only be an issue in a situation where some code expects to be able to use SetDeadline to interrupt the connection, right? So for example if I made a custom net.Listener where listener.Accept() returns websocket.NetConns and then used that listener for http.Server.Serve(listener), and then there was a hijack request on one of the websocket.NetConns.

Is that basically right?

@nhooyr
Copy link
Contributor Author

nhooyr commented Apr 7, 2024

I don't remember if net/http calls SetDeadline to interrupt on hijack but yes if it does then you will not be able to use the wrapper as it currently is.

I do plan on upgrading it to use the Deadline methods on the underlying connection someday but I don't have an ETA.

@anderspitman
Copy link

Sounds good, thanks!

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

Successfully merging a pull request may close this issue.

5 participants