Skip to content

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Mar 24, 2025

Fixes #113834.

3 distinct types of test failures

  • InvalidOperationException while writing to StreamBuffer - test implementation detail, replaced by real TCP streams
  • ODE and other exceptions wrapped in AuthenticationException - I couldn't decide whether to fix this in product, but then discovered that there were also other failures where there were weird error codes from platform crypto layer, so ignoring AuthenticationExceptions seems better here.

@Copilot Copilot AI review requested due to automatic review settings March 24, 2025 11:54
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

The PR aims to address SslStreamDisposeTest failures by using real TCP streams and by improving exception handling during parallel handshakes.

  • Replaced ConnectedSslStreams with real Tcp streams to avoid issues with concurrent disposal.
  • Updated the using blocks to reflect the new stream instantiation.
  • Expanded the exception catch logic to include AuthenticationException for more robust test validation.
Comments suppressed due to low confidence (2)

src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs:119

  • [nitpick] Consider renaming the 'server' variable to 'serverSslStream' for clarity and consistency with the client SslStream variable.
using SslStream server = new SslStream(serverStream);

src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs:151

  • Consider adding a dedicated test that simulates an AuthenticationException scenario to verify that this expected case is handled correctly.
catch (Exception ex) when (ex
                    is ObjectDisposedException // disposed locally
                    or IOException // disposed remotely (received unexpected EOF)
                    or AuthenticationException) // disposed wrapped in AuthenticationException or error from platform library

Copy link
Member Author

@rzikm rzikm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/azp run runtime-libraries-coreclr outerloop

@rzikm rzikm requested a review from a team March 24, 2025 11:55
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while the platform noise is unpleased IMHO I feel it is OK since we may be breaking operations in-flight. We would probably need to check disposed flag on failure to see that the failure is result of Dispose and throw ODE instead.

@rzikm
Copy link
Member Author

rzikm commented Mar 24, 2025

/azp run runtime-libraries-coreclr outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rzikm
Copy link
Member Author

rzikm commented Mar 25, 2025

all System.Net.Security.Tests pass, good to merge.

@rzikm rzikm merged commit 8be8425 into dotnet:main Mar 25, 2025
81 of 92 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 24, 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.

2 participants