Skip to content
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

[wasm] WebSocket improve send/receive buffers #58199

Merged
merged 1 commit into from
Sep 18, 2021

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Aug 26, 2021

  • Eliminate as many copies of the data as possible, copy data synchronously when possible
  • move buffers to js side
  • move most of the implementation to js
  • when send buffer became larger than 64k, resolve "send" Task only after data departed to OS via bufferedAmount === 0
  • resolve "close" Task only after we received "close" response from browser
  • introduce simple cancelable promise on JS side
  • Abort() cancels all the pending send/receive/open/close Tasks.
  • Abort() sends "close" to server, but doesn't wait for response
  • The cancelationToken cancels only the current operation and sets the whole WS into Aborted state. Other pending operations would finish normally but BrowserWebSocket would throw on any subsequent operations, except "close".

The previous BrowserWebSocket implementation did following actions, which the proposed implementation doesn't do

  • SendAsync didn't block task on endOfMessage for large pending transmission
  • CloseOutputAsync self produced onClose message for it's receivers
  • ConnectAsync sent "close" message to server and self produced onClose message for it's receivers, when canceled
  • ReceiveAsync sent "close" message to server , when canceled

Fixes #44614
Fixes #58202
Makes #57519 to just report slow test to console instead of fail.
Makes #53957 to just report slow test to console instead of fail.
Fixes #58031

@pavelsavara pavelsavara added arch-wasm WebAssembly architecture area-System.Net labels Aug 26, 2021
@ghost
Copy link

ghost commented Aug 26, 2021

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

Issue Details
  • Eliminate as many copies of the data as possible, copy data synchronously
  • resolve Task/promise only after data departed to OS via bufferedAmount === 0
  • throw when "Exactly one send and one receive is supported on each object in parallel." is breached

Fixes #44614

Author: pavelsavara
Assignees: -
Labels:

arch-wasm, area-System.Net

Milestone: -

@pavelsavara pavelsavara changed the title [DRADT][wasm] WebSocket improve send buffers [DRAFT][wasm] WebSocket improve send buffers Aug 26, 2021
@pavelsavara
Copy link
Member Author

Right now it doesn't handle all the abort/cancelation/dispose scenarios. Work in progress ... stay tuned :-D

@pavelsavara pavelsavara marked this pull request as ready for review September 1, 2021 16:31
@pavelsavara
Copy link
Member Author

I tested with Chrome, Edge and Firefox locally.

@pavelsavara pavelsavara requested a review from kg September 1, 2021 16:40
@kg
Copy link
Member

kg commented Sep 2, 2021

Why does this PR remove Add/RemoveEventListener?

@pavelsavara
Copy link
Member Author

Why does this PR remove Add/RemoveEventListener?

Because we don't need it anymore. Do you think that it should be part of the public API going forward ?

@pavelsavara
Copy link
Member Author

pavelsavara commented Sep 2, 2021

The implementation in the PR right now is running unit test suite 5x slower than main branch. It's caused by blocking send operation until the buffer is empty.

We had chat with @kg and here are conclusions

  • we would not block send operation's Task if the bufferedAmount is under 64k. This would improve throughput nearly to previous implementation and also apply backpressure for apps sending large data.
  • we would keep the Add/RemoveEventListener code
  • we would not automatically send 'close' to the server on operation cancelation. But we should allow the user to use CloseAsync after any cancelation.

Edit: this is all done now

@pavelsavara pavelsavara marked this pull request as draft September 7, 2021 21:58
@pavelsavara pavelsavara marked this pull request as ready for review September 8, 2021 11:48
@pavelsavara pavelsavara changed the title [DRAFT][wasm] WebSocket improve send buffers [wasm] WebSocket improve send/receive buffers Sep 8, 2021
Copy link
Member

@kg kg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good other than comments (you don't need to fix all of them)

@pavelsavara
Copy link
Member Author

Test failures are unrelated

@pavelsavara
Copy link
Member Author

pavelsavara commented Sep 15, 2021

Notes I made during code walkthrough with @CarnaViire

  • OutOfMemory exception, it should not be translated as WebSocketException
  • order of response ints[] on receive, name the index constants
  • rename promise_control for open
  • ws[BINDING.wasm_ws_pending_receive_promise_queue].drain -> int[]
  • fixed GC root for middle of array, use ArraySegment and offset instead
  • _mono_wasm_web_socket_send_buffering message_ptr-> message_root
  • utf8 test outerloop on coreCLR
  • bufferedAmount: "This value does not reset to zero when the connection is closed" what about CloseSent ?
  • assert: when we get message and the receivers queue is not empty, there need to be empty even queue,
  • assert: mirror of above in the receiver method
  • code 1000 on abort https://developer.mozilla.org/en-US/docs/Web/API/CloseEvent/code
    • 1005/1006 "is a reserved value and MUST NOT be set as a status code in a Close control frame by an endpoint." RFC6455
    • Chrome exception on using 1001: "The code must be either 1000, or between 3000 and 4999", same for no (optional) code
  • SendReceive_VaryingLengthBuffers_Success - no change for CoreCLR, keep the active issue for why it's so slow

THIRD-PARTY-NOTICES.TXT Outdated Show resolved Hide resolved
@pavelsavara
Copy link
Member Author

Follow up on yesterday conversation with @CarnaViire about CloseOutput from client side.
There is no such API on browser WS.
And therefore we call WebSocket.close() instead.
Chrome immediately issues 'close' event and it would not call and any 'message' callbacks after that, even if there were some messages in flight from the server.

So, we are losing the messages from server after CloseOutputAsync, as compared to managed/desktop implementation which delivers them.

The alternative is to not call WebSocket.close() when CloseOutputAsync, but that seems even worse.

@kg
Copy link
Member

kg commented Sep 16, 2021

Follow up on yesterday conversation with @CarnaViire about CloseOutput from client side.
There is no such API on browser WS.
And therefore we call WebSocket.close() instead.
Chrome immediately issues 'close' event and it would not call and any 'message' callbacks after that, even if there were some messages in flight from the server.

So, we are losing the messages from server after CloseOutputAsync, as compared to managed/desktop implementation which delivers them.

The alternative is to not call WebSocket.close() when CloseOutputAsync, but that seems even worse.

Hm, this is a huge failure on browser vendors' part. I don't like the data loss here. Is there some way we could signal this behavior difference to the developer?

@pavelsavara
Copy link
Member Author

The other differences in behavior are

  • we are not streaming partial messages, we delay them till endOfMessage.
  • we are sending close(1000) NormalClosure on Abort()

@pavelsavara
Copy link
Member Author

Is there some way we could signal this behavior difference to the developer?

We agreed with @kg to issue warning into console once per app lifetime.
WARNING: Web browsers do not support closing the output side of a WebSocket. CloseOutputAsync has closed the socket and discarded any incoming messages.

- CC0 notice for JavaScript queue
- feedback
@pavelsavara pavelsavara merged commit 360df71 into dotnet:main Sep 18, 2021
@karelz karelz added this to the 7.0.0 milestone Sep 22, 2021
CarnaViire added a commit that referenced this pull request Oct 8, 2021
Non-browser WS server used for ConnectAsync_Cancel_ThrowsCancellationException doesn't support "delay20sec", only "delay10sec". This basically reverts the change in this line from #58199 for non-browser platforms.

Contributes to #58355
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
@pavelsavara pavelsavara deleted the wasm_ws_send_buffers branch January 4, 2022 13:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Net
Projects
None yet
5 participants