Conversation
|
Tagging subscribers to this area: @dotnet/ncl |
6ba321b to
af5467f
Compare
There was a problem hiding this comment.
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
WebSocketStreamand related specialized stream types (ReadWriteStream,WriteMessageStream,ReadMessageStream) - Adds comprehensive tests in
WebSocketStreamTests.csand integrates them into the test project - Extends
ManagedWebSocketto 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.cswhich 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
WebSocketStreamconstructor, but the implementation only defines a constructor taking aWebSocketparameter. 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
ConfigureAwaitusesConfigureAwaitOptions.SuppressThrowing, butConfigureAwaitexpects abool. Replace withConfigureAwait(false)(ortrueif 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
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. |
CarnaViire
left a comment
There was a problem hiding this comment.
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.
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
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