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

QuicConnection.ConnectAsync raises SocketException when host not found #78751

Closed
Tracked by #82262
bentoi opened this issue Nov 23, 2022 · 10 comments
Closed
Tracked by #82262

QuicConnection.ConnectAsync raises SocketException when host not found #78751

bentoi opened this issue Nov 23, 2022 · 10 comments
Assignees
Labels
area-System.Net.Quic documentation Documentation bug or enhancement, does not impact product or test code
Milestone

Comments

@bentoi
Copy link

bentoi commented Nov 23, 2022

Description

The QuicConnect.ConnectAsync method raises a SocketException(SocketErrorCode = HostNotFound) if the resolution of a DnsEndPoint set with QuicClientOptions.RemoteEndpoint fails. I did expect a QuicException with a suitable error code instead.

Reproduction Steps

await using var connection = await QuicConnection.ConnectAsync(new QuicClientConnectionOptions
    {
          RemoteEndPoint = new DnsEndPoint("bogus.foo.com", 5001);
          ClientAuthenticationOptions = new SslClientAuthenticationOptions { ... } 
    });

Expected behavior

A QuicException with suitable QuicError.

Actual behavior

It raises a SocketException.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 23, 2022
@ghost
Copy link

ghost commented Nov 23, 2022

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

Issue Details

Description

The QuicConnect.ConnectAsync method raises a SocketException(SocketErrorCode = HostNotFound) if the resolution of a DnsEndPoint set with QuicClientOptions.RemoteEndpoint fails. I did expect a QuicException with a suitable error code instead.

Reproduction Steps

await using var connection = await QuicConnection.ConnectAsync(new QuicClientConnectionOptions
    {
          RemoteEndPoint = new DnsEndPoint("bogus.foo.com", 5001);
          ClientAuthenticationOptions = new SslClientAuthenticationOptions { ... } 
    });

Expected behavior

A QuicException with suitable QuicError.

Actual behavior

It raises a SocketException.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

Author: bentoi
Assignees: -
Labels:

area-System.Net.Quic

Milestone: -

@ManickaP ManickaP removed the untriaged New issue has not been triaged by the area owner label Nov 23, 2022
@ManickaP ManickaP added this to the 8.0.0 milestone Nov 23, 2022
@ManickaP
Copy link
Member

We have several places where we don't raise QuicException. Some raise AuthenticationException for example. However, it might be unexpected to get SocketException, seemingly System.Net.Sockets exclusive type, from System.Net.Quic.
On the other hand, in #76435 we got a feedback that we could leverage SocketException, albeit as a wrapped exception.
Lastly, in #75184, we have another use case that expects us to always wrap in QuicException.

Either way, we have several loosely related issue to exception types and their wrapping. We should take all this feedback and come up with a comprehensive solution to this.

Triage: this should be settled in 8.0.

cc: @rzikm

@wfurt
Copy link
Member

wfurt commented Nov 23, 2022

It seems like we don't document what ConnectAsync can throw. We should probably fix that regardless of the choice so callers can handle that properly.

@alexrp
Copy link
Contributor

alexrp commented Nov 24, 2022

It seems like we don't document what ConnectAsync can throw.

Just for the record, that seems to be a pervasive issue with all the QUIC API documentation, not just this particular method.

@antonfirsov
Copy link
Member

antonfirsov commented Nov 24, 2022

Note that we shouldn't document stored (async) exceptions in the exception section of the API docs - dotnet/dotnet-api-docs#7840 (comment). The fact that we do it for some of the networking API docs is a violation of general BCL API doc practices.

@alexrp
Copy link
Contributor

alexrp commented Nov 24, 2022

Note that we shouldn't document stored (async) exceptions in the exception section of the API docs - dotnet/dotnet-api-docs#7840 (comment). The fact that we do it for some of the networking API docs is a violation of general BCL API doc practices.

That issue mentions that async APIs should point to sync APIs for the possible exceptions. There are no sync APIs for QUIC at this time, though?

@antonfirsov
Copy link
Member

Yepp, just pointed it out in the discussion.

@ManickaP
Copy link
Member

ManickaP commented Jan 19, 2023

Triage: this is part of bigger revision on exception in Quic. We should address this 8.0.

Slightly related: #75184

@wfurt
Copy link
Member

wfurt commented Jun 30, 2023

We agreed to throw SocketException for transport errors - part of #82262 and based on API review feedback.

e.g. QuicExecption will indicate erros at Quic layer, SocketException with indicate problems bellow - e.g. IP or UDP. We shall document that somehow, perhaps as remarks because of pending debate about exceptions thrown synchronously vs exceptions stored in resulting Task.

@wfurt wfurt added the documentation Documentation bug or enhancement, does not impact product or test code label Jun 30, 2023
@ManickaP
Copy link
Member

ManickaP commented Oct 3, 2023

Closing this in lieu of #92943

@ManickaP ManickaP closed this as completed Oct 3, 2023
@karelz karelz modified the milestones: 8.0.0, 9.0.0 Oct 11, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Nov 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Quic documentation Documentation bug or enhancement, does not impact product or test code
Projects
None yet
Development

No branches or pull requests

6 participants