Skip to content

Perform WebSocket Close handshake in Conn.Close #104

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

Closed
wants to merge 1 commit into from

Conversation

nhooyr
Copy link
Contributor

@nhooyr nhooyr commented Jul 5, 2019

Closes #103

@stephenyama Could you please review?

@nhooyr nhooyr force-pushed the close-handshake branch from 3157a19 to b892206 Compare July 5, 2019 06:59
@nhooyr
Copy link
Contributor Author

nhooyr commented Jul 5, 2019

This won't work as is due to the limitation that you must be reading in another goroutine for the peer's close frame to be detected.

You might have an error in the reading goroutine and then call Close() to write a close frame. E.g. the JSON was invalid. You can't have Close block on waiting for a close frame from the client as no goroutine is reading from the connection.

Not sure the best way to solve this.

@stephenyama
Copy link

If the application decides to initiate the handshake from the reading goroutine, then the application must start a goroutine to call Close. This seems complicated.

@nhooyr
Copy link
Contributor Author

nhooyr commented Jul 5, 2019

Yea no I agree. It's not going to be merged as is.

I think if Close is called, we take the read lock and write all read data messages to ioutil.Discard until we find the close frame for a max 10 seconds.

This seems pretty reasonable.

I'm not sure if the Readering goroutine should block on obtaining the read lock or if it should error out immediately as it cannot read from the connection now that the closing handshake has started.

@stephenyama
Copy link

I think if Close is called, we take the read lock and write all read data messages to ioutil.Discard until we find the close frame for a max 10 seconds.

Discarding the message is no different from closing the connection immediately. Either way, the application cannot receive messages until the peer is done sending.

@nhooyr
Copy link
Contributor Author

nhooyr commented Jul 5, 2019

Discarding the message is no different from closing the connection immediately. Either way, the application cannot receive messages until the peer is done sending.

It is different as once the close frame has been received, you know the peer received all of your messages. If you're sending a close frame, why would you still want to receive messages from the peer?

The RFC says

   If an endpoint receives a Close frame and did not previously send a
   Close frame, the endpoint MUST send a Close frame in response.  (When
   sending a Close frame in response, the endpoint typically echos the
   status code it received.)  It SHOULD do so as soon as practical.  An
   endpoint MAY delay sending a Close frame until its current message is
   sent (for instance, if the majority of a fragmented message is
   already sent, an endpoint MAY send the remaining fragments before
   sending a Close frame).  However, there is no guarantee that the
   endpoint that has already sent a Close frame will continue to process
   data.

@nhooyr
Copy link
Contributor Author

nhooyr commented Jul 5, 2019

@stephenyama What would you want the library to do instead?

@stephenyama
Copy link

With your latest proposal, the application can determine if the peer received all messages. What's missing is a way to receive any responses to those messages.

A closing handshake is not unique to WebSockets. One example is the HTTP/1.1 Connection: close request and response headers.

How about something like this?

// CloseWrite sends a close message to the peer and sets a deadline for read on 
// the connection. The application should continue reading the connection until
// an error is returned (because the peer closed the connection, the deadline
// expired or a networking error).
func (c *Conn) CloseWrite(ctx context.Context, code StatusCode, reason string) error {}

// Close sends a close message to the peer (only if a message was not previously sent)
// and closes the network connection. 
func (c *Conn) Close(ctx context.Context, code StatusCode, reason string) error {}

@nhooyr
Copy link
Contributor Author

nhooyr commented Jul 12, 2019

With your latest proposal, the application can determine if the peer received all messages. What's missing is a way to receive any responses to those messages.

There is no guarantee that the peer will respond to any of those messages. It's explicitly stated in the RFC that you shouldn't rely on it.

The problem with your API proposal is that sending a close frame isn't really the same as closing the write side of the connection, it's saying I'm not going to be sending any more messages and I don't want to receive any more messages either and will close this connection.

@nhooyr
Copy link
Contributor Author

nhooyr commented Jul 12, 2019

Don't think I'll be moving forward with this. See discussion in #103

@nhooyr nhooyr closed this Jul 12, 2019
@nhooyr nhooyr deleted the close-handshake branch July 12, 2019 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement close handshake
2 participants