BUG/MEDIUM: mux_quic: adjust qcc_is_dead() to account detached streams

Muxes are responsible to release connections once they are inactive and
won't be reusable. In QUIC mux, such connections are detected via
qcc_is_dead(). The first precondition is that there is no more upper
streams attached. This was accounted via QCC <nb_sc> counter.

A special characteristic of QCS instances is that they can be in
detached state : upper stream has been removed but there is still data
to emit. Such QCS were not taken into account in qcc_is_dead(), so a
connection could be freed with some remaining data not yet emitted.

It is also not possible for QUIC MUX to simply look at the QCS tree to
determine if the connection is inactive. Indeed, some streams are opened
for protocol internal usage. This is the case for example with HTTP/3
unidirectional control stream or QPACK encoder/decoder streams. These
streams are never closed. In the end, only requests streams should be
taken into account for the connection activity.

This patch improves the situation by reworking <nb_hreq> QCC counter.
Previously, it served for http-request timeout implementation. However,
this timeout only relies on <opening_list> now. Thus, <nb_hreq> scope is
changed : it is now incremented via qcs_wait_http_req(), used by app
protocol layer once a request stream is identified. Decrement is
performed on qcs_free(), so this guarantees that a connection cannot be
freed anymore if request streams still exists, unless if inactivity
timeout fires. As such, <nb_hreq> now supersedes <nb_sc> entirely, so
the qcc_is_dead() can now relies on the former.

Along with this change, qcc_timeout_task() must be updated. Call to
qcc_is_dead() was unnecessary prior to this patch as timeout handling
was only active when no upper streams were attached. When tested, both
<nb_sc> and QCC <task> were already null, so a connection was always
released on timeout, as expected. With qcc_is_dead() now checking
<nb_hreq> instead, this is not always the case anymore. In fact, this
check is unnecessary as inactivity timeout serves precisely to free a
stucked connection with remaining data to emit.

This patch also has some impact on http-keep-alive timeout. Previously,
this timeout could be armed if only detached streams remained. Now, it
is only applicable if all QCS request instances are closed and freed.
Thus, qcc_reset_idle_start() is now closed directly on qcs_free().

Ideally this should be backported up to 2.6, or at least 2.8 as QUIC
experimental status was removed there.

diff --git a/include/haproxy/mux_quic.h b/include/haproxy/mux_quic.h
index 232dfe7d2103dde6c0302354e1a1d89e5b9f1f2b..00f245c617ab0436a98402414f142ee8003b7d8b 100644 (file)
--- a/include/haproxy/mux_quic.h
+++ b/include/haproxy/mux_quic.h
@@ -98,9 +98,10 @@ static inline char *qcs_st_to_str(enum qcs_state st)
 
 int qcc_install_app_ops(struct qcc *qcc);
 
-/* Register <qcs> stream for http-request timeout. If the stream is not yet
- * attached in the configured delay, qcc timeout task will be triggered. This
- * means the full header section was not received in time.
+/* Flags <qcs> as a request stream. The connection will be considered as active
+ * until all request streams are closed or on inactivity timeout. On the
+ * frontend side, http-request timeout will be applied on the stream to ensure
+ * headers are received in time.
  *
  * This function should be called by the application protocol layer on request
  * streams initialization.
@@ -109,6 +110,9 @@ static inline void qcs_wait_http_req(struct qcs *qcs)
 {
        struct qcc *qcc = qcs->qcc;
 
+       /* For frontend connections, register the stream in QCC opening_list.
+        * This is necessary for http-request timeout.
+        */
        if (!conn_is_back(qcc->conn)) {
                /* A stream cannot be registered several times. */
                BUG_ON_HOT(tick_isset(qcs->start));
@@ -119,6 +123,11 @@ static inline void qcs_wait_http_req(struct qcs *qcs)
                 */
                LIST_APPEND(&qcc->opening_list, &qcs->el_opening);
        }
+
+       /* Ensure flag is only set once per stream to avoid nb_hreq counter wrapping. */
+       BUG_ON_HOT(qcs->flags & QC_SF_HREQ_RECV);
+       qcs->flags |= QC_SF_HREQ_RECV;
+       ++qcc->nb_hreq;
 }
 
 void qcc_show_quic(struct qcc *qcc);

diff --git a/src/mux_quic.c b/src/mux_quic.c
index ac798e475ea634c07ff749f34a64ebfa493d33cd..29d17947d7c5cd2ea7b81179ce34df93e8de6607 100644 (file)
--- a/src/mux_quic.c
+++ b/src/mux_quic.c
@@ -102,6 +102,11 @@ static void qcs_free(struct qcs *qcs)
        sedesc_free(qcs->sd);
        qcs->sd = NULL;
 
+       if (qcs->flags & QC_SF_HREQ_RECV) {
+               BUG_ON(!qcc->nb_hreq);
+               --qcc->nb_hreq;
+       }
+
        /* Release app-layer context. */
        if (qcs->ctx && qcc->app_ops->detach)
                qcc->app_ops->detach(qcs);
@@ -270,44 +275,25 @@ static forceinline void qcc_rm_sc(struct qcc *qcc)
 {
        BUG_ON(!qcc->nb_sc); /* Ensure sc count is always valid (ie >=0). */
        --qcc->nb_sc;
-
-       /* Reset qcc idle start for http-keep-alive timeout. Timeout will be
-        * refreshed after this on stream detach.
-        */
-       if (!conn_is_back(qcc->conn) && qcc_may_expire(qcc) && !qcc->nb_hreq)
-               qcc_reset_idle_start(qcc);
-}
-
-/* Decrement <qcc> hreq. */
-static forceinline void qcc_rm_hreq(struct qcc *qcc)
-{
-       BUG_ON(!qcc->nb_hreq); /* Ensure http req count is always valid (ie >=0). */
-       --qcc->nb_hreq;
-
-       /* Reset qcc idle start for http-keep-alive timeout. Timeout will be
-        * refreshed after this on I/O handler.
-        */
-       if (!conn_is_back(qcc->conn) && qcc_may_expire(qcc) && !qcc->nb_hreq)
-               qcc_reset_idle_start(qcc);
 }
 
 static inline int qcc_is_dead(const struct qcc *qcc)
 {
-       /* Maintain connection if stream endpoints are still active. */
-       if (qcc->nb_sc)
+       /* Maintain connection if there is still request streams active. */
+       if (qcc->nb_hreq)
                return 0;
 
        /* Connection considered dead if either :
         * - remote error detected at transport level
         * - error detected locally
         * - MUX timeout expired
-        * - app layer shut and all transfers done (FE side only - used for stream.max-total)
+        * - app layer shut (FE side only - used for stream.max-total)
         * - new stream initiating definitely blocked (BE side only - used for H3 GOAWAY reception)
         */
        if (qcc->flags & (QC_CF_ERR_CONN|QC_CF_ERRL_DONE) ||
            !qcc->task ||
-           (!conn_is_back(qcc->conn) && !qcc->nb_hreq && qcc->app_st == QCC_APP_ST_SHUT) ||
-           (conn_is_back(qcc->conn) && !qcc->nb_hreq && (qcc->flags & QC_CF_CONN_SHUT))) {
+           (!conn_is_back(qcc->conn) && qcc->app_st == QCC_APP_ST_SHUT) ||
+           (conn_is_back(qcc->conn) && (qcc->flags & QC_CF_CONN_SHUT))) {
                return 1;
        }
 
@@ -450,9 +436,6 @@ void qcs_close_local(struct qcs *qcs)
 
        if (quic_stream_is_bidi(qcs->id)) {
                qcs->st = (qcs->st == QC_SS_HREM) ? QC_SS_CLO : QC_SS_HLOC;
-
-               if (qcs->flags & QC_SF_HREQ_RECV)
-                       qcc_rm_hreq(qcs->qcc);
        }
        else {
                /* Only local uni streams are valid for this operation. */
@@ -1044,13 +1027,9 @@ int qcs_attach_sc(struct qcs *qcs, struct buffer *buf, char fin)
                return -1;
        }
 
-       /* QC_SF_HREQ_RECV must be set once for a stream. Else, nb_hreq counter
-        * will be incorrect for the connection.
-        */
-       BUG_ON_HOT(qcs->flags & QC_SF_HREQ_RECV);
-       qcs->flags |= QC_SF_HREQ_RECV;
+       /* QCS must be identified as request stream prior to stconn instantiation. */
+       BUG_ON(!(qcs->flags & QC_SF_HREQ_RECV));
        ++qcc->nb_sc;
-       ++qcc->nb_hreq;
        ++qcc->tot_sc;
 
        /* TODO duplicated from mux_h2 */
@@ -2535,6 +2514,12 @@ static void qcs_destroy(struct qcs *qcs)
 
        qcs_free(qcs);
 
+       /* Rearm http-keep-alive timeout when last request stream is freed. */
+       if (!conn_is_back(qcc->conn) && qcc_may_expire(qcc) && !qcc->nb_hreq) {
+               qcc_reset_idle_start(qcc);
+               qcc_refresh_timeout(qcc);
+       }
+
        TRACE_LEAVE(QMUX_EV_QCS_END, conn);
 }
 
@@ -3784,11 +3769,9 @@ static struct task *qcc_timeout_task(struct task *t, void *ctx, unsigned int sta
         * shutdown should occurs. For all other cases, an immediate close
         * seems legitimate.
         */
-       if (qcc_is_dead(qcc)) {
-               TRACE_STATE("releasing dead connection", QMUX_EV_QCC_WAKE, qcc->conn);
-               qcc_app_shutdown(qcc);
-               qcc_release(qcc);
-       }
+       TRACE_STATE("releasing dead connection", QMUX_EV_QCC_WAKE, qcc->conn);
+       qcc_app_shutdown(qcc);
+       qcc_release(qcc);
 
  out:
        TRACE_LEAVE(QMUX_EV_QCC_WAKE);