Add WebSocketStream #114363
Add WebSocketStream #114363
Conversation
ghost
added
|
Note regarding the |
1 similar comment
|
Note regarding the |
|
Tagging subscribers to this area: @dotnet/ncl |
| if (WebSocket.State is WebSocketState.Open) | ||
| { | ||
| // There's no synchronous close, so we're forced to do sync-over-async. | ||
| WebSocket.CloseOutputAsync(WebSocketCloseStatus.NormalClosure, null, CancellationToken.None).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
Wouldn't this cause deadlocks in UI contexts such as WPF and Windows Forms?
There was a problem hiding this comment.
Not as long as everything within CloseOutputAsync uses ConfigureAwait(false)
| { | ||
| if (WebSocket.State is WebSocketState.Open) | ||
| { | ||
| await WebSocket.CloseOutputAsync(WebSocketCloseStatus.NormalClosure, null, CancellationToken.None).ConfigureAwait(false); |
There was a problem hiding this comment.
Is it reasonable to use CancellationToken.None here? Since we're disposing the websocket if the remote party fails to respond to the Close request and we don't have a keep alive set, we will basically hang here.
Would it not be better if we have some timeout for the close to complete?
There was a problem hiding this comment.
Yes. The close timeout is one of the open questions at the moment. The full list is in #111217 (comment) -- we'd appreciate your thoughts on it as well 🙏
There was a problem hiding this comment.
From an API standpoint I would prefer if the close timeout can be specified in WebSocketCreationOptions much like the KeepAliveTimeout. Doing so eliminates the need for the consumers of the WebSocket to know about it. I would even go further than this, making the CloseTimeout have a meaningful value by default.
|
Closing until design concerns are addressed |
Closes #111217