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.
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.
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.
Thanks! I have made additional modifications to the code. Please check it. :)
There was a problem hiding this comment.
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_nsthat also readsquiche_conn_timeout()for setting a Curl_expire
There was a problem hiding this comment.
-
That's right, using a variable to record the value of timeout can improve performance. However, in order to remain consistent with
curl_ngtcp2.cand minimize code modifications, I chose not to do this. Maybe we can consider optimizing the implementations ofcurl_ngtcp2andcurl_quichetogether in the future. -
Reading
quiche_conn_timeoutagain 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_timeoutonly 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_timeoutwhen 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.