Skip to content

Conversation

@antonfirsov
Copy link
Contributor

@antonfirsov antonfirsov commented Aug 3, 2023

Fixes #89095
Fixes #89096

Applies the heuristic solutions we agreed on.

@antonfirsov antonfirsov added this to the 8.0.0 milestone Aug 3, 2023
@antonfirsov antonfirsov requested a review from wfurt August 3, 2023 17:34
@ghost ghost assigned antonfirsov Aug 3, 2023
@ghost
Copy link

ghost commented Aug 3, 2023

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

Issue Details

Fixes #89095, fixes #89096 by applying the heuristic solutions we agreed on.

Author: antonfirsov
Assignees: -
Labels:

area-System.Net

Milestone: 8.0.0

Comment on lines +38 to +41
IgnoreScopeIdIPEndpointComparer endPointComparer = new();
Assert.Equal(listener.LocalEndPoint, serverConnection.LocalEndPoint, endPointComparer);
Assert.Equal(listener.LocalEndPoint, clientConnection.RemoteEndPoint, endPointComparer);
Assert.Equal(clientConnection.LocalEndPoint, serverConnection.RemoteEndPoint, endPointComparer);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To improve confidence in my changes, I extended this test to test IPv6. serverConnection.LocalEndPoint has a ScopeId for some reason while listener.LocalEndpoint doesn't. I assume it is asigned after binding, and we can ignore this difference.

Copy link
Member

Choose a reason for hiding this comment

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

scopeID should be only relevant for LLA. I'm wondering if we are deviating from Socket/NetworkStream here because the underlying MSQuic implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's possible. If we think it's a problem we can track it in a separate issue.

Copy link
Member

Choose a reason for hiding this comment

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

It seems like this was existing condition. We miss it because QuicConnectionTests.TestConnect only tests IPv4. I sync back and I tried it with IPv6 and it also fails:

   Starting:    System.Net.Quic.Functional.Tests (parallel test collections = on, max threads = 4)
      System.Net.Quic.Tests.QuicConnectionTests.TestConnect [STARTING]
      System.Net.Quic.Tests.QuicConnectionTests.TestConnect [FAIL]
        Assert.Equal() Failure
        Expected: [::1]:34850
        Actual:   [::1%1]:34850

I will fix it tomorrow.
(cc: @ManickaP)

Comment on lines +155 to +156
if (exception is SocketException socketException &&
socketException.SocketErrorCode is SocketError.HostNotFound or SocketError.TryAgain)
Copy link
Member

Choose a reason for hiding this comment

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

If you wanted to, since you're already using pattern matching, you could make this even more concise:

exception is SocketException { SocketErrorCode: SocketError.HostNotFound or SocketError.TryAgain }

@karelz
Copy link
Member

karelz commented Aug 4, 2023

Failures were unrelated in System.Text.Encoding.Tests on Ubuntu 18.04, rerunning them.

  • System.Text.Tests.AsciiEqualityTests_Byte_Char.EqualsIgnoreCase_EqualValues_ButNonAscii_ReturnsFalse - 33x
  • System.Text.Tests.AsciiEqualityTests_Char_Char.EqualsIgnoreCase_EqualValues_ButNonAscii_ReturnsFalse - 33x
  • System.Text.Tests.AsciiEqualityTests_Char_Char.Equals_EqualValues_ButNonAscii_ReturnsFalse - 33x
  • System.Text.Tests.AsciiEqualityTests_Char_Byte.EqualsIgnoreCase_EqualValues_ButNonAscii_ReturnsFalse - 33x

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.

LGTM

@wfurt wfurt merged commit a1b2535 into dotnet:main Aug 4, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 10, 2023
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.

[HttpRequestError] Detect NameResolutionError more reliably Detect name resolution errors in QuicConnection.ConnectAsync

5 participants