Skip to content

Conversation

@stephentoub
Copy link
Member

Fixes #111217

@dotnet-policy-service
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 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
@github-actions github-actions bot locked and limited conversation to collaborators Jul 26, 2025
@stephentoub stephentoub deleted the websocketstream2 branch December 12, 2025 23:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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