Throw exception with correct QuicError for ConnectionTimeout #73227
Conversation
ghost
assigned 
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFixes #73141. Looks like I missed one QuicError member when mapping MsQuic statuses to exceptions. Now they are all correctly mapped.
|
| @@ -68,6 +68,7 @@ static Exception GetExceptionInternal(int status, string? message) | |||
| if (status == QUIC_STATUS_ADDRESS_IN_USE) return new QuicException(QuicError.AddressInUse, null, SR.net_quic_address_in_use); | |||
| if (status == QUIC_STATUS_UNREACHABLE) return new QuicException(QuicError.HostUnreachable, null, SR.net_quic_host_unreachable); | |||
| if (status == QUIC_STATUS_CONNECTION_REFUSED) return new QuicException(QuicError.ConnectionRefused, null, SR.net_quic_connection_refused); | |||
| if (status == QUIC_STATUS_CONNECTION_TIMEOUT) return new QuicException(QuicError.ConnectionTimeout, null, SR.net_quic_timeout); | |||
There was a problem hiding this comment.
This change looks fine, but subsequently I think this method would be cleaner if implemented as a switch.
There was a problem hiding this comment.
the QUIC_STATUS_* members are not compile-time constants, unfortunately, they are get-only properties with if-else per platform (they come from autogenerated .cs files from MsQuic)
There was a problem hiding this comment.
they are get-only properties with if-else per platform (they come from autogenerated .cs files from MsQuic)
😮
There was a problem hiding this comment.
Do we know why msquic defines different values for these constants per platform? That sounds like a very leaky abstraction.
There was a problem hiding this comment.
It tries to conform to the usual platform error codes: HRESULTs on Windows, ERRNOs on Unixes
There was a problem hiding this comment.
That's misguided from my perspective, but ok.
There was a problem hiding this comment.
There was a problem hiding this comment.
I think it's a design flaw that the library is surfacing platform-specific details like this... it doesn't need to in a case like this, and is just pushing the pain upwards to consumers of the library.
There was a problem hiding this comment.
I think it's a design flaw that the library is surfacing platform-specific details like this... it doesn't need to in a case like this, and is just pushing the pain upwards to consumers of the library.
I am not thrilled about it either, but at least now they give us autogeneated stubs which contain the right numbers so we can be reasonably sure that we did not typo it.
I am not sure how to test this, this gets thrown when MsQuic fails to receive an ACK for some time and decides that the connection is dead. |
Is that only on packet loss? I would expect that one can send idle timeout and it would surface if there is no activity. |
As far as I can tell from the MsQuic source, yes, it is when we expect an ACK from a peer and it does not arrive within some time window (16s by default). It could also apply when attempting a new connection and the receiver is not replying with anything (not even ICMP refusal).
We already have tests for |
|
I tried to create a test for connection timeout when establishing a new connection: [Fact]
// [OuterLoop("Can take a long time")]
public async Task ConnectAsync_ServerNotResponding_ThrowsConnectTimeoutException()
{
// Open a socket to make the target host reachable
using Socket s = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);
s.Bind(new IPEndPoint(IPAddress.Loopback, 0));
await AssertThrowsQuicExceptionAsync(QuicError.ConnectionTimeout, async () =>
await CreateQuicConnection((IPEndPoint)s.LocalEndPoint));
}But this fails with ConnectionIdle because during handshake, there is a smaller timeout (10s) for connection going idle. This value is configurable, but we don't expose that API yet (the API is proposed in #72984). So we don't have a way to test for this at the moment without significant investment. |
|
The test failure is unrelated |
Fixes #73141.
Looks like I missed one QuicError member when mapping MsQuic statuses to exceptions. Now they are all correctly mapped.