Skip to content

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Apr 1, 2025

Fixes #113833.

Previous fix was not enough, lots of opened sockets caused additional failures. This PR changes back to in-memory streams and filters out the InvalidOperationException which happens only in those cases.

@Copilot Copilot AI review requested due to automatic review settings April 1, 2025 13:33
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 fixes issues related to the parallel handshake on Unix by switching to in-memory streams and refining error filtering.

  • Replace TCP streams with in-memory streams
  • Update error handling to filter out InvalidOperationException during concurrent disposal

Copy link
Contributor

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

…eamDisposeTest.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@rzikm rzikm requested a review from a team April 1, 2025 13:34
@wfurt
Copy link
Member

wfurt commented Apr 1, 2025

It is curious that the memory stream throes InvalidOperationException when NetworkStream does not? Or is that really Invalid access we do in SslStream?

@rzikm
Copy link
Member Author

rzikm commented Apr 2, 2025

well, the parallel dispose call disposes the underlying StreamBuffer, causing the InvalidException when attempting to write the next chunk to underlying stream

private (bool wait, int bytesWritten) TryWriteToBuffer(ReadOnlySpan<byte> buffer)
{
Debug.Assert(buffer.Length > 0);
Debug.Assert(!Monitor.IsEntered(SyncObject));
lock (SyncObject)
{
if (_writeEnded)
{
throw new InvalidOperationException();
}

Since the StreamBuffer is just some internal helper, it does not distinguish between state where only write side was closed or the entire instance got disposed.

In NetworkStream, we would get ObjectDisposedException or similar

@rzikm
Copy link
Member Author

rzikm commented Apr 4, 2025

/ba-g Ci failures are unrelated

@rzikm rzikm merged commit 9b024f6 into dotnet:main Apr 4, 2025
81 of 87 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators May 4, 2025
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.

[Test Failure] SslStreamDisposeTest.Dispose_ParallelWithHandshake_ThrowsODE on Unix

2 participants