-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
quiche: fix quiche being unable to handle timeout events properly #11654
Conversation
Set the `QUIC_IDLE_TIMEOUT` parameter to match ngtcp2 for consistency.
In parallel with ngtcp2, quiche also offers the `quiche_conn_on_timeout` interface for the application to invoke upon timer expiration. Therefore, invoking the `on_timeout` function of the Connection is crucial to ensure seamless functionality of quiche with timeout events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR goes in the right direction. But needs to be built on to invoke quiche_conn_on_timeout()
only in case the timeout really did happen.
lib/vquic/curl_quiche.c
Outdated
@@ -751,6 +751,12 @@ static CURLcode cf_flush_egress(struct Curl_cfilter *cf, | |||
struct read_ctx readx; | |||
size_t pkt_count, gsolen; | |||
|
|||
quiche_conn_on_timeout(ctx->qconn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This calls the function unconditionally on every egress handling. Which is not what the quiche example seem to mandate (guessing through their lack of documentation).
I think we need to handle timeouts more elaborate here. But we can do this based on this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I have made additional modifications to the code. Please check it. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few things:
- Calling quiche_conn_on_timeout() before any timeout elapsed is fine, there is no harm other than wasted cycles.
- It would be better to grab the timeout value once and only fire when expires, than query it every time and check for 0 (optimization)
- its not clear to me how this relates to the existing
timeout_ns
that also readsquiche_conn_timeout()
for setting a Curl_expire
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
That's right, using a variable to record the value of timeout can improve performance. However, in order to remain consistent with
curl_ngtcp2.c
and minimize code modifications, I chose not to do this. Maybe we can consider optimizing the implementations ofcurl_ngtcp2
andcurl_quiche
together in the future. -
Reading
quiche_conn_timeout
again is to promptly update the timing for the next timeout. After processing withquiche_conn_on_timeout
, the timeout duration might change. Perhaps I can make a small optimization by placing the update of timeout and the setting of a Curl_expire after callingquiche_conn_on_timeout
, which would require queryingquiche_conn_timeout
only when necessary. But this may cause a little more code modifications.
@icing you think we should merge this as-is and consider further improvements later? It seems like a step in the right direction at least to me. |
I agree. Let's merge it. |
Thanks! |
In parallel with ngtcp2, quiche also offers the `quiche_conn_on_timeout` interface for the application to invoke upon timer expiration. Therefore, invoking the `on_timeout` function of the Connection is crucial to ensure seamless functionality of quiche with timeout events. Closes curl#11654
According to quiche's document, it is essential to invoke
quiche_conn_on_timeout
when a timer expires. Failing to call this function can result in quiche being unable to manage various timeout events, including scenarios like retransmitting lost packets and handling idle timeout events.