Skip to content
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

Harden IPv6Any error handling #45529

Merged
merged 2 commits into from
Dec 16, 2022
Merged

Harden IPv6Any error handling #45529

merged 2 commits into from
Dec 16, 2022

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented Dec 9, 2022

Fixes flaky test #43548

This test starts the server and expect a NotSupportedException due to some TLS options we don't support with HTTP/3(Quic).

Buggy flow:

  • Bind to IPv6Any for Sockets / HTTP1 & 2
  • Bind to IPv6Any for Quic, throw NotSupportedException when setting up TLS
  • AnyIPListenOptions catches the Exception and tries to rebind to IPv4Any for Sockets instead. However, we were just on that port so we'll often get a conflict.

Fix: Don't fall back to IPv4Any for a NotSupportedException.

I also cleaned up some log statements and adjusted the port scanning to skip around more and avoid conflicts with other tests.

@adityamandaleeka
Copy link
Member

cc @amcasey

@Tratcher
Copy link
Member Author

@JamesNK ?

Comment on lines +28 to +29
// HttpsConnectionMiddleware.CreateHttp3Options, Http3 doesn't support OnAuthenticate.
&& ex is not NotSupportedException)
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a strong guarantee, i.e. someone could change the exception thrown without realizing they change behavior here.

Is there test coverage for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, there's this test 😁

Copy link
Member

Choose a reason for hiding this comment

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

Is this the behavior we want for all NotSupportedExceptions? If not, would it make more sense to throw a subtype?

Copy link
Member Author

@Tratcher Tratcher Jan 9, 2023

Choose a reason for hiding this comment

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

We avoid defining custom exception types without an explicit need. We can reconsider if we find a conflict. We could compare the messages if we're paranoid.

src/Servers/Kestrel/shared/test/ServerRetryHelper.cs Outdated Show resolved Hide resolved
Co-authored-by: James Newton-King <james@newtonking.com>
@Tratcher Tratcher enabled auto-merge (squash) December 15, 2022 23:36
@Tratcher Tratcher merged commit 1eb9766 into dotnet:main Dec 16, 2022
@Tratcher Tratcher deleted the tratcher/ipv6any branch December 16, 2022 01:20
@ghost ghost modified the milestones: Test failures, 8.0-preview1 Dec 16, 2022
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants