Hi Amaury,
Thanks for taking a look.
> I'm not sure to agree yet with the proposed fix as this could have
> unwanted consequences, most notably with the backend support (which
> the proposed patch does not support here). However, a localized fix
> in qcc_recv_reset_stream() is at least mandatory here.
That's a fair point, and honestly the reason our patch doesn't cover
qcc_be_is_reusable()/backend semantics is simply that we don't run any
QUIC/HTTP3 backends ourselves, so we hadn't considered that case at
all.
We're currently carrying a live leak in production from this (around
1.22M stuck connections on one node before we restarted it), so we're
planning to apply both what we submitted before (the qcc_is_dead()
reorder, frontend-only as submitted) and the change below internally,
regardless of the outcome of the wider upstream discussion. Since
we don't run QUIC/HTTP3 backends, the backend concern you raised
doesn't affect our own deployment either way.
The change below is the qcc_recv_reset_stream() timeout refresh you
mentioned as at least mandatory. It isn't guarded by conn_is_back() at
all, so it already applies uniformly to both sides. For reference,
this is what we have (against current master):
int qcc_recv_reset_stream(struct qcc *qcc, uint64_t id, uint64_t err,
uint64_t final_size)
{
...
qcs->flags |= QC_SF_SIZE_KNOWN|QC_SF_RECV_RESET;
qcs_close_remote(qcs);
+ /* Unlike STREAM frame handling and qcc_recv_stop_sending(), this
+ * function had no qcc_refresh_timeout() call of its own. A stream
+ * reset before its request is complete leaves nb_hreq stuck and
+ * never emits any TX data, so qcc->task could otherwise be left
+ * with a stale expire computed once at qmux_init() time and never
+ * queued. qcc_refresh_timeout() already checks qcc_may_expire()
+ * (based on nb_sc, not nb_hreq) and recomputes from opening_list,
+ * so it is safe to call unconditionally here.
+ */
+ qcc_refresh_timeout(qcc);
+
/* RFC 9000 3.2. Receiving Stream States
*
* An
* implementation MAY interrupt delivery of stream data, discard any
* data that was not consumed, and signal the receipt of the
* RESET_STREAM.
...
Wanted to check first in case you see any problem with us applying
both of these internally ahead of whatever ends up merged upstream, or
any risk specific to a frontend-only QUIC deployment that we might be
missing beyond the backend point you already raised.
