Skip to content

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

Merged
merged 8 commits into from
Jun 26, 2025
Merged

Add WebSocketStream #116729

merged 8 commits into from
Jun 26, 2025

Conversation

stephentoub
Copy link
Member

Fixes #111217

Copy link
Contributor

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

Copy link
Contributor

@Copilot Copilot AI left a 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 a WebSocket 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 uses ConfigureAwaitOptions.SuppressThrowing, but ConfigureAwait expects a bool. Replace with ConfigureAwait(false) (or true if appropriate) to match the API.
                                await WebSocket.CloseAsync(WebSocketCloseStatus.NormalClosure, null, ct).ConfigureAwait(ConfigureAwaitOptions.SuppressThrowing);

@stephentoub
Copy link
Member Author

@CarnaViire, any feedback? Thoughts on @MihaZupan's question?

@CarnaViire
Copy link
Member

@CarnaViire, any feedback? Thoughts on @MihaZupan's question?

Sorry for the delay. I will bulk post the review today.

Copy link
Member

@CarnaViire CarnaViire left a 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!!

if (!_disposed)
{
_disposed = true;
return WebSocket.SendAsync(ReadOnlyMemory<byte>.Empty, _messageType, endOfMessage: true, CancellationToken.None);
Copy link
Member

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?

@CarnaViire
Copy link
Member

@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.

For read message stream, disposing is equal to cancelling a read operation
@CarnaViire CarnaViire enabled auto-merge (squash) June 26, 2025 02:05
@CarnaViire CarnaViire merged commit ba14314 into dotnet:main Jun 26, 2025
82 of 87 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Add APIs to WebSocket which allow it to be read as a Stream
3 participants