Review closing the connection

[najamelan]

In a similar idea of #76 I would like to have a look at this part of WebSocketContext::write_pending:

// If we're closing and there is nothing to send anymore, we should close the connection.
if self.role == Role::Server && !self.state.can_read() {
    // The underlying TCP connection, in most normal cases, SHOULD be closed
    // first by the server, so that it holds the TIME_WAIT state and not the
    // client (as this would prevent it from re-opening the connection for 2
    // maximum segment lifetimes (2MSL), while there is no corresponding
    // server impact as a TIME_WAIT connection is immediately reopened upon
    // a new SYN with a higher seq number). (RFC 6455)
    self.state = WebSocketState::Terminated;
    Err(Error::ConnectionClosed)
} else {
    Ok(())
}

Note that right now the code still does a check on WebSocketState::ClosedByPeer | WebSocketState::CloseAcknowledged, but that's exactly what can_read covers, namely that we received a close frame from the remote, so I cleaned that up a bit.

This code mixes 2 concerns:

• dealing with closing the underlying connection, which is earlier for server than client. As far as I can tell from the code, we only set state to Terminated when it's safe to drop the underlying connection, and when we do that we return ConnectionClosed to indicate that to client code.
• moving our state forward because now the close handshake is finished. It stands out now that it's kind of strange that we check whether we can read even though we are in write_pending

What causes this I think is that there is no state in WebSocketState to indicate the close handshake is finished when the remote initiated it. CloseAcknowlegded is only used when we initiated. We know that when we received an initial close frame from the remote, we will have cued a reply so this kind of bumps it to terminated, but only for server role.

It doesn't really make sense to have CloseAcknowlegded only count when we initiated the close handshake. The websocket protocol mainly distinguishes a state when the handshake is finished, regardless of who initiated it. I think it's just the way it is because given the design of tungstenite, it's kind of hard to do this right. We know when we cue a close response, but we never know when we actually sent it out. It seems not easy to fix this without creating extra overhead on each call to write_pending, similar to how it's done for pong messages for example.

There is a few problems:

• what do we do for clients? Currently nothing because we have no state to represent this as we're not allowed to use terminated yet, to avoid dropping the connection to soon and there is no state to say the close handshake is finished. This leads to an error that we allow to continue sending even after the close handshake is finished (for clients). Do not allow sending messages after sending a close frame #74 tries to address this for both clients and servers (which also still could send whilst waiting for a close acknowledge), but it's not yet clear what error type that should return. Do not allow sending messages after sending a close frame #74 solves that without touching the above code.
• the documentation for AlreadyClosed claims that you will get AlreadyClosed if you try to send after receiving a Message::Close. For servers we can see here that we will actually return ConnectionClosed if the user tries to send after receiving a close. Integration test: already_closed.rs.txt.
• we claim to be terminated, and thus ready to drop. However there is nothing here that guarantees that the close frame has actually been sent out. There might be back pressure from the underlying connection and it might still be sitting in the queue. In that case we return ConnectionClosed to soon, because we shouldn't drop the underlying connection yet.

[daniel-abramov]

This code mixes 2 concerns:

I think it's only one concern, the first one in your list, but since it's not explicitly coded as a state machine, it looks confusing. write_pending() is called both from read and from write functions, so having it inside read_message() ensures that we "write pending data" even if the user does not explicitly send anything, thus we can ensure that the state of the websocket connection is consistent and can be used even if you have a "read-only client" which only receives some messages and does not explicitly write anything. Without having write_pending() inside we would have to force the user to always call write_pending() explicitly which does not seem to be good.

As far as I can tell from the code, we only set state to Terminated when it's safe to drop the underlying connection, and when we do that we return ConnectionClosed to indicate that to client code.

Correct.

moving our state forward because now the close handshake is finished. It stands out now that it's kind of change that we check whether we can read even though we are in write_pending

You will get inside this block in the following scenario: you're using the library on the server side. At one point you decide to close the connection and send a close frame to the user. The user replies to it. The execution will then get to this part of write_sending() and change the state to Terminated and returns ConnectionReset. An attempt to call read_message() one more time will result in getting AlreadyClosed.

There was also some test written for that:

• dbc8bd7
• later changed in e2bec4b

[daniel-abramov]

There is a few problems:
what do we do for clients? Currently nothing because we have no state to represent this as we're not allowed to use terminated yet, to avoid dropping the connection to soon and there is no state to say the close handshake is finished.

The connection on the client side will be terminated after server closes the socket.

the documentation for AlreadyClosed claims that you will get AlreadyClosed if you try to send after receiving a Message::Close. For servers we can see here that we will actually return ConnectionClosed if the user tries to send after receiving a close.

AlreadyClosed is only returned when you try to work with the already closed connection. It's only returned after you've once received ConnectionClosed.

we claim to be terminated, and thus ready to drop. However there is nothing here that guarantees that the close frame has actually been sent out. There might be back pressure from the underlying connection and it might still be sitting in the queue. In that case we return ConnectionClosed to soon, because we shouldn't drop the underlying connection yet.

No, you'll only get to that part of code when the send queue is empty (that's also documented in a comment above that block of code).

[najamelan]

AlreadyClosed is only returned when you try to work with the already closed connection. It's only returned after you've once received ConnectionClosed.

That's just a documentation error. Just note that it is possible to receive a Protocol error followed by AlreadyClosed without ever receiving ConnectionClosed. From here.

No, you'll only get to that part of code when the send queue is empty (that's also documented in a comment above that block of code).

I'm sorry, that's my bad. In that case, there isn't an actual error here. So I'll close this issue.

[daniel-abramov]

That's just a documentation error. Just note that it is possible to receive a Protocol error followed by AlreadyClosed without ever receiving ConnectionClosed. From here.

Yeah, I mean... of course if there is a protocol violation (a bug on one of the sides), then you can get this error instead of ConnectionClosed. But it does not seem to be an issue. AlreadyClosed is just an attempt to work with an already closed connection (i.e. it's some error or bug on a user side that they try to use the object when it's literally not-usable anymore). After all these discussions I feel that it might make sense to roll back to the state when we only had ConnectionClosed error and there was no confusion about non-existing AlreadyClosed.