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

[QUIC&H/3] QuicConnection handshake timeout #104358

Closed
wants to merge 1 commit into from

Conversation

ManickaP
Copy link
Member

@ManickaP ManickaP commented Jul 3, 2024

Increased QuicConnection handshake timeout in tests.

Optional: propagated SocketsHttpHandler.ConnectTimeout to QuicConnectionOptions.HandshakeTimeout. Note that since the default is "infinite", this might lead to hanging connection establishment. So if there are any concerns, I'll revert this part of the change.

Copy link
Contributor

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

@ManickaP ManickaP requested a review from a team July 3, 2024 12:01
@liveans
Copy link
Member

liveans commented Jul 3, 2024

Optional: propagated SocketsHttpHandler.ConnectTimeout to QuicConnectionOptions.HandshakeTimeout. Note that since the default is "infinite", this might lead to hanging connection establishment. So if there are any concerns, I'll revert this part of the change.

I think this should be fine, because in the end HttpClient will fall back to the original Timeout.

@MihaZupan
Copy link
Member

Setting the timeout to match SocketsHttpHandler.ConnectTimeout should be functionally the same as disabling it outright as the ConnectTimeout already controls the cancellation token passed to Quic.

Given that we don't have a default ConnectTimeout so far, I would actually prefer that we keep the current handshake timeout of 10 seconds (seems like plenty of time), unless it's been showing up as problematic?

@ManickaP
Copy link
Member Author

ManickaP commented Jul 3, 2024

because in the end HttpClient will fall back to the original Timeout.

What original timeout?

@ManickaP
Copy link
Member Author

ManickaP commented Jul 3, 2024

Given that we don't have a default ConnectTimeout so far, I would actually prefer that we keep the current handshake timeout of 10 seconds (seems like plenty of time), unless it's been showing up as problematic?

Some tests in CI are timing out on HandshakeTimeout. Before this PR, there's no way to change the default 10 seconds through HttpClient. But I do concur that 10s should be enough in all cases, for some reason it isn't. @liveans can you share the logs, kusto query, etc about the failing tests on handshake timeout?

@liveans
Copy link
Member

liveans commented Jul 3, 2024

because in the end HttpClient will fall back to the original Timeout.

What original timeout?

What I meant was HttpClient.Timeout property

@ManickaP
Copy link
Member Author

ManickaP commented Jul 4, 2024

because in the end HttpClient will fall back to the original Timeout.

What original timeout?

What I meant was HttpClient.Timeout property

That doesn't apply to connection creation. Also not present when using message invoker directly.

@@ -123,6 +123,7 @@ public static async ValueTask<QuicConnection> ConnectQuicAsync(HttpRequestMessag
MaxInboundBidirectionalStreams = 0, // Client doesn't support inbound streams: https://www.rfc-editor.org/rfc/rfc9114.html#name-bidirectional-streams. An extension might change this.
MaxInboundUnidirectionalStreams = 5, // Minimum is 3: https://www.rfc-editor.org/rfc/rfc9114.html#unidirectional-streams (1x control stream + 2x QPACK). Set to 100 if/when support for PUSH streams is added.
IdleTimeout = idleTimeout,
HandshakeTimeout = handshakeTimeout,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this could just be Timeout.InfiniteTimeSpan

The cancellationToken passed to ConnectAsync is already backed by ConnectTimeout. Making the handshake timeout the same value may make it a coin flip whether you see a handshake timeout vs. connect timeout exception.

@ManickaP
Copy link
Member Author

ManickaP commented Jul 8, 2024

I'm still waiting for more data/info on this. As the 10 seconds should be enough even in CI. So the question is why are we seeing the handshake timeouts? We need to understand the problem better.
Also this has the unfortunate effect of making the handshake timeout infinite by default even on the transport level. How do sockets behave in this regard? Do they have some inherent OS level timeout?

Either way, I'm making this draft for now, potentially closing in the future.

@ManickaP ManickaP marked this pull request as draft July 8, 2024 14:13
Copy link
Contributor

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@karelz karelz added this to the 9.0.0 milestone Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants