- 
                Notifications
    You must be signed in to change notification settings 
- Fork 5.2k
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 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 WebSocketStreamimplementation | 
| src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs | Extracted ThrowIfInvalidMessageTypehelper | 
| src/libraries/System.Net.WebSockets/src/Resources/Strings.resx | Added net_WebSockets_TimeoutOutOfRangestring | 
| 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 WebSocketStreambehavior | 
| 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. | 
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
          
            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