WebSocket and multiple reads and writes

[CarnaViire]

It seems that after #56282 ManagedWebSocket started to allow multiple reads and writes by ordering them inside via AsyncMutex. This seems to go against our docs that say "Exactly one send and one receive is supported on each WebSocket object in parallel." Before that PR, at least read side did the check and called ThrowIfOperationInProgress - now it's not called anywhere.

We have 2 tests called ReceiveAsync_MultipleOutstandingReceiveOperations_Throws and SendAsync_MultipleOutstandingSendOperations_Throws, but they are implemented in a way that if an exception was thrown, it checked to be an expected one. If no exceptions were thrown, the test would succeed.

We need to either re-introduce the check, or decide that we are happy with current behavior and update docs accordingly.

/cc @pavelsavara @stephentoub

Failures 6/27-8/27 (incl. PRs):

Day	Run	Test
7/16	4x PR #55849 - Mono wasm	ReceiveAsync_MultipleOutstandingReceiveOperations_Throws
7/18	PR #55889 - Mono wasm	ReceiveAsync_MultipleOutstandingReceiveOperations_Throws
7/27	Official run - Mono wasm	ReceiveAsync_MultipleOutstandingReceiveOperations_Throws
8/25	PR #57745 - Mono wasm	ReceiveAsync_MultipleOutstandingReceiveOperations_Throws
8/25	Official run - release/6.0 - Mono wasm	ReceiveAsync_MultipleOutstandingReceiveOperations_Throws

Wasm failures are:

Assert.Equal() Failure
 Expected: Aborted
 Actual:   Closed

State immediately after ConnectAsync incorrect: Closed
Expected: True
Actual:   False

Closed state when OperationCanceledException
Expected: True
Actual:   False

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

It seems that after #56282 ManagedWebSocket started to allow multiple reads and writes by ordering them inside via AsyncMutex. This seems to go against our docs that say "Exactly one send and one receive is supported on each WebSocket object in parallel." Before that PR, at least read side did the check and called ThrowIfOperationInProgress - now it's not called anywhere.

We have 2 tests called ReceiveAsync_MultipleOutstandingReceiveOperations_Throws and SendAsync_MultipleOutstandingSendOperations_Throws, but they are implemented in a way that if an exception was thrown, it checked to be an expected one. If no exceptions were thrown, the test would succeed.

We need to either re-introduce the check, or decide that we are happy with current behavior and update docs accordingly.

/cc @pavelsavara @stephentoub

Author:	CarnaViire
Assignees:	-
Labels:	area-System.Net, untriaged
Milestone:	-

[pavelsavara]

I found this issue while working on improvements to WASM here #58199
It would be good for me to know the expected behavior soon.

From my WASM perspective it would be good to stick to "Exactly one send and one receive is supported on each WebSocket object in parallel."
Because browser WebSocket.send() doesn't block and we have to poll WebSocket.bufferedAmount to know what was already passed to OS. That makes resolving different Tasks/promises on different parallel messages quite difficult.

Changed my mind

[pavelsavara]

cc @lewing @karelz

[stephentoub]

We need to either re-introduce the check, or decide that we are happy with current behavior and update docs accordingly.

From my perspective, both the docs and the ManagedWebSocket implementation are good as-is (albeit we can delete a dead function and clean up some tests).

The docs are correct: only one receive and one send are supported concurrently. But there are many things that are unsupported that happen to just work sometimes. It's unsupported to use a List<T>'s Add method concurrently from multiple threads, even though it'll probably work (or appear to work) most of the time if you do it. It's not worth it adding extra overhead to try to throw an exception in the case of such misuse. This case with ManagedWebSocket is similar.

Note that in .NET 5 it already "just worked" to have concurrent sends, again even though such use would be unsupported. The implementation had to enable concurrent sends in order to support a keep-alive timer firing ping requests, and so while unsupported, concurrent sends "just worked", as it wasn't worthwhile trying to distinguish valid use from invalid use. You can see that the cited "ThrowIfOperationInProgress" method only has one call site, on the receive side. The way the implementation was tracking an active receive in order to be able to raise that error was expensive. And it's not worth it paying that expense to flag unsupported concurrent use.

Note, however, that we have supported CloseAsync and CloseOutputAsync concurrently with both ReceiveAsync and SendAsync. I don't know if anyone's depended on that, nor do I know if it's documented, but technically we made it work, both previously and now. (CloseAsync needs to both send and receive, and CloseOutputAsync needs to send.)

[ghost]

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

---

It seems that after #56282 ManagedWebSocket started to allow multiple reads and writes by ordering them inside via AsyncMutex. This seems to go against our docs that say "Exactly one send and one receive is supported on each WebSocket object in parallel." Before that PR, at least read side did the check and called ThrowIfOperationInProgress - now it's not called anywhere.

We have 2 tests called ReceiveAsync_MultipleOutstandingReceiveOperations_Throws and SendAsync_MultipleOutstandingSendOperations_Throws, but they are implemented in a way that if an exception was thrown, it checked to be an expected one. If no exceptions were thrown, the test would succeed.

We need to either re-introduce the check, or decide that we are happy with current behavior and update docs accordingly.

/cc @pavelsavara @stephentoub

Failures 6/27-8/27 (incl. PRs):

Day	Run	Test
7/16	4x PR #55849 - Mono wasm	ReceiveAsync_MultipleOutstandingReceiveOperations_Throws
7/18	PR #55889 - Mono wasm	ReceiveAsync_MultipleOutstandingReceiveOperations_Throws
7/27	Official run - Mono wasm	ReceiveAsync_MultipleOutstandingReceiveOperations_Throws
8/25	PR #57745 - Mono wasm	ReceiveAsync_MultipleOutstandingReceiveOperations_Throws
8/25	Official run - release/6.0 - Mono wasm	ReceiveAsync_MultipleOutstandingReceiveOperations_Throws

Wasm failure is

Assert.Equal() Failure
 Expected: Aborted
 Actual:   Closed

Author:	CarnaViire
Assignees:	-
Labels:	arch-wasm, area-System.Net, untriaged
Milestone:	-

[pavelsavara]

Thanks @stephentoub and @CarnaViire.
I changed my mind and will implement WASM WS.send also as not guarding parallel.
I still need do more work on receive side before final conclusion.
Also I'm hoping that I could also fix those failures mentioned in head post.