Skip to content

Commit

Permalink
BUG/MEDIUM: quic_conn: let the scheduler kill the task when needed
Browse files Browse the repository at this point in the history
The two timer handlers qc_process_timer() and qc_idle_timer_task() would
inadvertently return NULL when they don't want to be requeued, instead
of just returning the task itself. The effect of returning NULL for the
scheduler is that it considers the task as freed, so it must not touch
it anymore. As such, the TASK_F_RUNNING flag is never removed from these
tasks, and when quic_conn_release() later tries to release these tasks
using task_destroy(), the latter sees the RUNNING flag and just sets
->process to NULL, hoping that the scheduler will kill them on return,
but there's no longer being executed so this never happens and they are
leaked.

Interestingly, this doesn't seem to happen as much when multi-queue is
set to off, but it's likely because the tasks are being replaced and the
first ones have already been woken up and leaked, while the latter might
only trigger on a timeout or timer renewal.

This should address github issue haproxy#2310. Thanks to @hpn0t0ad for the
numerous traces that helped understand this sequence.

This must be backported to 2.7 at least, and adapted for 2.6
(qc_idle_timer_task must return t there).

(cherry picked from commit 4070e40)
Signed-off-by: Willy Tarreau <[email protected]>
  • Loading branch information
wtarreau committed Oct 17, 2023
1 parent 106008b commit ebb226f
Showing 1 changed file with 1 addition and 5 deletions.
6 changes: 1 addition & 5 deletions src/quic_conn.c
Original file line number Diff line number Diff line change
Expand Up @@ -5389,7 +5389,6 @@ struct task *qc_process_timer(struct task *task, void *ctx, unsigned int state)

if (qc->flags & (QUIC_FL_CONN_DRAINING|QUIC_FL_CONN_TO_KILL)) {
TRACE_PROTO("cancelled action (draining state)", QUIC_EV_CONN_PTIMER, qc);
task = NULL;
goto out;
}

Expand Down Expand Up @@ -5970,6 +5969,7 @@ struct task *qc_idle_timer_task(struct task *t, void *ctx, unsigned int state)
if (qc->mux_state != QC_MUX_READY) {
quic_conn_release(qc);
qc = NULL;
t = NULL;
}

/* TODO if the quic-conn cannot be freed because of the MUX, we may at
Expand All @@ -5982,10 +5982,6 @@ struct task *qc_idle_timer_task(struct task *t, void *ctx, unsigned int state)
HA_ATOMIC_DEC(&prx_counters->half_open_conn);
}

leave:
TRACE_LEAVE(QUIC_EV_CONN_IDLE_TIMER, qc);
return NULL;

requeue:
TRACE_LEAVE(QUIC_EV_CONN_IDLE_TIMER, qc);
return t;
Expand Down

0 comments on commit ebb226f

Please sign in to comment.