Runtime: WebSocket state difference when waiting for Close

[caesar-chen]

In .Net Core: The WebSocket is only supposed to transition to Closed when the connection is closed, e.g. as part of CloseAsync after a close frame has been both sent and received.

But in .Net Framework, if WebSocket is in CloseSent/CloseReceived state and receive/send a close packet, it will immediately transit into Close state (don't wait for CloseAsync). Causes test failure here: https://github.com/dotnet/corefx/blob/master/src/System.Net.WebSockets.Client/tests/CloseTest.cs#L285

[Diego-Perez-Botero]

The UWP ClientWebSocket implementation (WinRTWebSocket.cs) was modeled after .NET Framework and fails the same CloseTest test case in a similar way.

[Diego-Perez-Botero]

Based on the MSDN documentation, the .NET Framework behavior (also followed by UWP ClientWebSocket) may be the most accurate:

Closed | Indicates the WebSocket close handshake completed gracefully.
-- | --

[Diego-Perez-Botero]

Tl;DR: Non-UWP .NET Core should be fixed to adopt the same _WebSocketState.Closed_ behavior as .NET Framework.

From this morning's discussion:

1. ClientWebSocket sends out a close frame whenever CloseOutputAsync or CloseAsync are called.

2. When CloseOutputAsync is called in non-UWP .NET Core, the close frame is sent out and the websocket's state transitions to _CloseSent_, even if a close frame had already been received from the server before the CloseOutputAsync call (i.e., websocket's state was _CloseReceived_). If a close frame is received from the server after that, the websocket's state becomes _CloseReceived_, which doesn't make it clear whether or not a close frame had previously been sent by the client.

3. When CloseAsync is called in non-UWP .NET Core, the close frame is sent out (if it hasn't been sent yet) and the socket wait for the server's close frame to arrive (if it hasn't already). On success, the websocket's state becomes _Closed_.

Note that _Closed_ state is supposed to be dependent on the websocket close handshake's status. In .NET Framework and UWP .NET Core, (1) and (3) are the same. However, (2) is less ambiguous: after CloseOutputAsync, the websocket's state automatically transitions to _Closed_ if the server's close frame is received (or was received already). That way, the _Closed_ state's semantics are respected and there is no weird back-and-forth between _CloseReceived_ and _CloseSent_ states.

[stephentoub]

It's not clear to me from the description whether the change being proposed would only impact what's returned from properties on the WebSocket class or whether it would impact what's done on the wire. If the latter, have you validated this against a test framework like https://github.com/crossbario/autobahn-testsuite? I'd previously validated the managed implementation we use on Unix and which ASP.NET uses on all platforms and got it to 100% passing (not including the tests for things not implemented, like websockets compression). I don't know if this will impact it, but the suite had lots of tests around close behavior, and I know we had to make a few fixes to pass those tests, so I just want to make sure we don't regress there.

cc: @anurse

[Diego-Perez-Botero]

The change being proposed is limited to the _ClientWebSocketState_ transitions (i.e., property value returned to the app). What's being sent/received down the wire should remain as-is.

[davidsh]

+1 on the autobahn test suite. We used it extensively when we first implemented websocket protocol across many components in Windows and .NET Framework.

[anurse]

The .NET Framework behavior sounds like the correct behavior to me, and it shouldn't affect autobahn compliance. Obviously it should be re-verified after the change to ensure it remains compliant. Are the autobahn tests part of the automated tests for this component?

[dadude999]

Any updates on prioritization here? Scenario 2 as described by @Diego-Perez-Botero is pretty difficult to handle without throwing in extra flags to indicate whether the client already got a close request from the server:

When CloseOutputAsync is called in non-UWP .NET Core, the close frame is sent out and the websocket's state transitions to _CloseSent_, even if a close frame had already been received from the server before the CloseOutputAsync call (i.e., websocket's state was _CloseReceived_). If a close frame is received from the server after that, the websocket's state becomes _CloseReceived_, which doesn't make it clear whether or not a close frame had previously been sent by the client.

[davidsh]

@karelz

[davidsh]

The change being proposed is limited to the _ClientWebSocketState_ transitions (i.e., property value returned to the app). What's being sent/received down the wire should remain as-is.

As noted above, the fix needed in this issue in a fix to .NET Core System.Net.WebSockets.Client (non-UWP version). Specifically, it is a fix needed in the value of the state variable, _ClientWebSocketState_.

Any updates on prioritization here?

Due to the large number of backlogged issues, this issue has not been prioritized higher. But if you have a PR with a fix (and a test), we will gladly accept it.

[dadude999]

As noted above, the fix needed in this issue in a fix to .NET Core System.Net.WebSockets.Client (non-UWP version). Specifically, it is a fix needed in the value of the state variable, _ClientWebSocketState_.

Right, that's what I'm referring to as well.

Due to the large number of backlogged issues, this issue has not been prioritized higher. But if you have a PR with a fix (and a test), we will gladly accept it.

I'm afraid I can't do that for legal reasons related to my job. Otherwise I'd be happy to.

[davidsh]

This is fixed now as of PR dotnet/corefx#36975

[stephentoub]

@davidsh, there's still a test disabled against this:
https://github.com/dotnet/runtime/blob/04f22268efb4434b7cd81e8ae2c3354be99fd4a7/src/libraries/System.Net.WebSockets.Client/tests/CloseTest.cs#L263-L266