Handling close in a websocket proxy

[superfashi]

Support wanted for better coroutine manipulation.

[garyburd]

Describe the problem you are trying to solve in more detail.

[superfashi]

context package, as declared in standard packages and document available here https://golang.org/pkg/context/, is used to handle network requests and responses in goroutine. For example, if a connection holds a cancellable context, does the standard http package, and the context is canceled, the whole request will be canceled, including connecting, reading or writing, and the open socket will be closed.

Honestly, this is pretty self-explanatory if you know what context package does. Furthermore, the Dial param in http.Transport is already deprecated, and the DialContext is a great replacement for that, so I believe it should be the same for this websocket package as well.

[garyburd]

I am asking you to describe the problem you are trying to solve in your application, not a description of the context package and how the package is used elsewhere.

[superfashi]

For example, when I am dialing, instead of HandshakeTimeout I want to use context to cancel it.

[garyburd]

Why does the timeout not work for your application? Describe the problem.

I am not going to sprinkle magic context dust over the package with the hope that it will solve actual problems.

[superfashi]

As I said, context is created and suggested for problems like this. Let me give you a more detailed and realistic situation.

There's a MITM websocket service running, two goroutines respectively handle connections from (client to self) and (self to the real server).

Say that I am reading data from (self to the real server) connection and to send it to the client side, but since the real server is not sending anything, the procedure is blocked. Now, the client closed the (client to self) connection, so I am supposed to be closing (self to the real server) connection as well. However, since the (self to the real server) goroutine is still blocked on reading, I can do nothing about it.

Now I know that I can set a ReadTimeout on (self to the real server) side, and detect if the other (client to self) connection is still alive after every timeout, but a cancellable context would be a perfect and quick solution for this: simply calls cancel() and the reading is not blocked anymore and returns an error instead.

[garyburd]

Context cancelation is not the right solution for handling close in a websocket proxy. Let me know if this is an actual problem you are encountering. If it is, then I'lll take the time to describe how to handle close in a proxy.

I know what the context package is and the problems the package is intended to solve. I am willing to add context related features based on actual application requirements. I don't want to spend time reviewing or implementing features that nobody needs.

[superfashi]

Yes, please go ahead. My current solution is inserting detection anywhere in the code, which is really messy.
And would you please explain why a cancellable context is not the right solution?

[garyburd]

The RFC describes how to cleanly close a websocket connection.

I assume that you have two connections, serverConn and clientConn.

Create two channels for communicating that the connections are closed:

  serverClosed := make(chan struct{})
  clientClosed := make(chan struct{})

Run goroutines to copy the connections:

 go copyConn(serverConn, clientConn, serverClosed, clientClosed)
 go copyConn(clientConn, serverConn, clientClosed, serverClosed)

where copyConn is:

func copyConn(readConn, writeConn *websocket.Conn, readClosed, writeClosed chan struct{}) {
	var rerr error
	for {
		var r io.Reader
		var messageType int

		messageType, r, rerr = readConn.NextReader()
		if rerr != nil {
			break
		}
		w, err := writeConn.NextWriter(messageType)
		if err != nil {
			break
		}
		if _, err := io.Copy(w, r); err != nil {
			break
		}
                if err := w.Close(); err != nil {
                       break
               }
	}

	// Close the reading connection. If we broke out of the loop because of a
	// normal close, then NextReader echoed the close message and we should now
	// close the connection.  If it's an abnormal close, then we should give up
	// and close the connection.
	readConn.Close()

	// Tell the other goroutine that readConn was closed.
	close(readClosed)

	// Did we break out of the loop because we received a close message?
	if e, ok := rerr.(*websocket.CloseError); ok && e.Code != websocket.CloseAbnormalClosure {

		// Forward the close message to writeConn.
		var m []byte
		if e.Code != websocket.CloseNoStatusReceived {
			m = websocket.FormatCloseMessage(e.Code, e.Text)
		}
		err := writeConn.WriteMessage(websocket.CloseMessage, m)

		// Did we successfully send the close message?
		if err == nil {
			// Wait with timeout for the other goroutine to handle the handshake.
			select {
			case <-writeClosed:
				// The other goroutine closed writeConn.
				return
			case <-time.After(10 * time.Second):
			}
		}
	}

        // A blocked reader returns with an error when the connection is closed.
	writeConn.Close()
}

The goal of the code above is to show how to coordinate the closing handshake across two goroutines. A robust proxy application will use PING/PONG and deadlines to detect dead connections and peers.

The most complicated part of the code is related to closing handshake. If the handshake is dropped, the code reduces to for { ... copy one connection to the other and break on error ... } readConn.Close(); writeConn.Close().

And would you please explain why a cancellable context is not the right solution?

Coordinating the closing handshake between two connections is an application problem. The above code can be rewritten to use contexts instead of plain channels and timers, but that's overkill. In any case, it's a layer above the websocket package.

I know that I can set a ReadTimeout on (self to the real server) side, and detect if the other (client to self) connection is still alive after every timeout,

A connection is not usable after a timeout error. This does not work.

simply calls cancel() and the reading is not blocked anymore and returns an error instead.

Close the connection to cause a read to return with an error.

[superfashi]

That'll do, thanks.

[finsterdexter]

Apologies for commenting on an old closed issue, but I'm still learning Go, and the code above, while functional, seems less idiomatic than using context.Context, at least that's the impression I've gotten from the reading I've done (and implementing exactly what the OP was trying to do).

As I understand it, context.Context is created to solve exactly this kind of problem without having to create a bunch of signal channels to keep track of everything, and through the Context.Done() channel, you can control the running of all your own internal goroutines while keeping them synchronized with external resources that support context.Context. It's the same reason that CancellationTokens are used in .NET, for example. They're a really useful pattern for keeping disparate asynchronous systems in lock-step when everything needs to be closed down (due to error, disconnect, etc.)

I see that Context support has been merged in as of a few days ago, so maybe it's beyond moot at this point, but mainly I'm trying to be a better Go programmer and trying to understand the language features (goroutine, context.Context, channels, etc.) a bit better. Also, context.Context support will result in much cleaner code in my own implementation as I can clean up a lot of signal channels and just rely on <-context.Context.Done(), as well as the other features of context.Context like Deadlines. (As an aside, grpc recommends using Deadlines for all grpc connections as a best practice.)

Again, apologies for beating a bit of a dead horse, here.

[garyburd]

The OP's actual issue is implementing close in a websocket proxy. I updated the title of the issue to reflect this.

Coordinating the application's goroutines, sending close messages and and receiving close messages is the responsibility of the application in this scenario. This package provides all of the functionality required to implement this scenario idiomatically. Context support is not needed for the scenario.

Open a new issue if you require some functionality not present in this package.

[finsterdexter]

I guess what I'm really asking is for clarification about this statement:

Context cancelation is not the right solution for handling close in a websocket proxy.

In my reading that sounds like exactly what context cancellation was written to handle. I don't understand why one wouldn't want to use context cancellation (assuming context support were available throughout your service stack). Again, I'm just trying to understand the core concepts and why context is not considered idiomatic in this case.

[garyburd]

My statement refers to functionality implemented by this package.

The functionality needed from this package for proxy shutdown is: write a message to a connection, read a message from a connection and close a connection. These features are exposed as methods on the connection object. Exposing these features through context cancelation is not needed, nor does it make sense.

This is not an appropriate forum for learning about the context package.