-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Add WebSocketStream #116729
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
Add WebSocketStream #116729
Conversation
Tagging subscribers to this area: @dotnet/ncl |
6ba321b
to
af5467f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new WebSocketStream
abstraction to forward Stream
operations over a WebSocket
and includes accompanying tests, resource updates, and API surface adjustments.
- Introduces
WebSocketStream
and related specialized stream types (ReadWriteStream
,WriteMessageStream
,ReadMessageStream
) - Adds comprehensive tests in
WebSocketStreamTests.cs
and integrates them into the test project - Extends
ManagedWebSocket
to centralize message-type validation and updates resources and reference assemblies
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStream.cs | Added WebSocketStream implementation |
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs | Extracted ThrowIfInvalidMessageType helper |
src/libraries/System.Net.WebSockets/src/Resources/Strings.resx | Added net_WebSockets_TimeoutOutOfRange string |
src/libraries/System.Net.WebSockets/ref/System.Net.WebSockets.cs | Updated reference assembly for WebSocketStream |
src/libraries/System.Net.WebSockets/tests/System.Net.WebSockets.Tests.csproj | Updated test project to include new tests |
src/libraries/System.Net.WebSockets/tests/WebSocketStreamTests.cs | Added tests for new WebSocketStream behavior |
src/libraries/Common/tests/StreamConformanceTests/System/IO/StreamConformanceTests.cs | Updated conformance tests to async patterns |
Comments suppressed due to low confidence (4)
src/libraries/System.Net.WebSockets/src/Resources/Strings.resx:142
- [nitpick] The error message is unclear; consider rephrasing to "The timeout must be non-negative or Timeout.InfiniteTimeSpan." for better readability.
<value>The timeout must be a value between non-negative or Timeout.InfiniteTimeSpan.</value>
src/libraries/System.Net.WebSockets/tests/System.Net.WebSockets.Tests.csproj:11
- The project references
WebSocketCreateTest.cs
which does not exist; it should reference the actual test file (WebSocketStreamCreateTests.cs
) to include the new tests and avoid compilation failures.
<Compile Include="WebSocketCreateTest.cs" />
src/libraries/System.Net.WebSockets/ref/System.Net.WebSockets.cs:51
- The reference assembly declares a parameterless
WebSocketStream
constructor, but the implementation only defines a constructor taking aWebSocket
parameter. This mismatch will cause API and compilation inconsistencies.
private protected WebSocketStream() { }
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStream.cs:273
- The call to
ConfigureAwait
usesConfigureAwaitOptions.SuppressThrowing
, butConfigureAwait
expects abool
. Replace withConfigureAwait(false)
(ortrue
if appropriate) to match the API.
await WebSocket.CloseAsync(WebSocketCloseStatus.NormalClosure, null, ct).ConfigureAwait(ConfigureAwaitOptions.SuppressThrowing);
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStream.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStream.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStream.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStream.cs
Outdated
Show resolved
Hide resolved
6096911
to
d025c85
Compare
d025c85
to
f88f36f
Compare
…WebSocketStream.cs
@CarnaViire, any feedback? Thoughts on @MihaZupan's question? |
Sorry for the delay. I will bulk post the review today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general with couple of nits
Thanks!!
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStream.cs
Show resolved
Hide resolved
if (!_disposed) | ||
{ | ||
_disposed = true; | ||
return WebSocket.SendAsync(ReadOnlyMemory<byte>.Empty, _messageType, endOfMessage: true, CancellationToken.None); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this also have .ConfigureAwait(ConfigureAwaitOptions.SuppressThrowing);
to ensure it won't throw?
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStream.cs
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStream.cs
Outdated
Show resolved
Hide resolved
@stephentoub I'm going to apply the suggestion from #116729 (comment) to move forward with this. If there are any concerns, we can address them in a follow-up. |
src/libraries/System.Net.WebSockets/tests/WebSocketStreamTests.cs
Outdated
Show resolved
Hide resolved
For read message stream, disposing is equal to cancelling a read operation
Fixes #111217