From: Christopher Faulet Date: Tue, 10 Nov 2026 15:18:15 +0000 (+0100) Subject: BUG/MEDIUM: mux-fcgi: put BE conn back in idle list after deferred shut X-Git-Url: https://git.kaiwu.me/ngx_http_flv_module.html?a=commitdiff_plain;h=fea3cb41990194d9fbbef37867986e5d4777f237;p=haproxy.git BUG/MEDIUM: mux-fcgi: put BE conn back in idle list after deferred shut When a stream cannot send its FCGI_ABORT record or its end of stream at detach time because the mux buffer is busy or full, fcgi_detach() leaves early to keep the stream as an orphan. This skips the idle/avail list management performed at the end of the function. The orphan is later destroyed by fcgi_deferred_shut() once the record can be sent, but nothing makes the connection idle or available again at this point. The timeout is not refreshed either. If the orphan was the last stream of the connection, the connection is then in no idle list, it is not released and it remains accounted in srv->curr_used_conns. Its timeout is the one armed when the stream was orphaned, so the connection is closed after "timeout server" instead of being reused, and it is wasted in the mean time. To fix the issue, the idle/avail list management is extracted from fcgi_detach() into a new fcgi_may_idle_conn() function, which is now also called from fcgi_deferred_shut() after the orphan was destroyed and the timeout was refreshed, as fcgi_detach() does. In fcgi_deferred_shut(), the stream's session cannot be used, since it may have been released in the mean time. The connection owner is used instead, but only if the connection is still attached to its session, because session_free() resets conn->owner only in this case. For a private connection, if there is no session anymore when the last stream leaves, the connection can no longer be reused and it is destroyed. The orphans destroyed from fcgi_strm_wake_one_stream() are not covered here. This mostly happens on connection errors or shutdowns, in which case the connection is released anyway. This is the same fix as the one applied on mux-h2 by the commit "BUG/MEDIUM: mux-h2: put BE conn back in idle list after deferred shut". This could be backported to all supported versions, after the recent fcgi_detach() fixes: a167d39a5 ("BUG/MEDIUM: mux-fcgi: arm the connection timeout only when data remains"), 170a6469c ("BUG/MEDIUM: mux-fcgi: ignore conn->owner when deciding if a connection is dead"), b77b044d3 ("MINOR: mux-fcgi: check for a dead connection before refreshing the timeout") and 2acf116bf ("MINOR: mux-fcgi: don't add a closed connection to an idle list on detach"). --- diff --git a/src/mux_fcgi.c b/src/mux_fcgi.c index 028792e34..f3f4ef32c 100644 --- a/src/mux_fcgi.c +++ b/src/mux_fcgi.c @@ -3788,6 +3788,106 @@ static void fcgi_destroy(void *ctx) } } +/* Performs the idle/avail list management of a backend connection after one + * of its streams was destroyed. It is called from fcgi_detach(), and from + * fcgi_deferred_shut() for orphaned streams that were blocked at detach time. + * The connection must not be dead and its timeout must already have been + * refreshed. is the session owning the connection if it is private. It + * may be NULL if the session is already gone, in which case an idle private + * connection cannot be reused anymore and is destroyed. + * + * Returns 1 if the connection was moved to an idle list or destroyed, in which + * case the caller must not use anymore. Otherwise 0 is returned. + */ +static int fcgi_may_idle_conn(struct fcgi_conn *fconn, struct session *sess) +{ + if ((fconn->flags & (FCGI_CF_EOS|FCGI_CF_ERR_PENDING|FCGI_CF_ERROR)) || + (fconn->state == FCGI_CS_CLOSED) || + !(fconn->flags & FCGI_CF_KEEP_CONN)) + return 0; + + if (fconn->conn->flags & CO_FL_PRIVATE) { + if (!sess) { + if (!eb_is_empty(&fconn->streams_by_id)) + return 0; + + /* The session is gone and the connection is idle, nobody can reuse it */ + CALL_MUX_NO_RET(fconn->conn->mux, destroy(fconn)); + TRACE_DEVEL("outgoing connection killed after its session", FCGI_EV_STRM_END); + return 1; + } + + /* Add the connection in the session serverlist, if not already done */ + session_add_conn(sess, fconn->conn); + + if (eb_is_empty(&fconn->streams_by_id)) { + if (!LIST_INLIST(&fconn->conn->sess_el)) { + /* Session insertion above has failed and connection is idle, remove it. */ + CALL_MUX_NO_RET(fconn->conn->mux, destroy(fconn)); + TRACE_DEVEL("outgoing connection killed", FCGI_EV_STRM_END|FCGI_EV_FCONN_ERR); + return 1; + } + + /* mark that the tasklet may lose its context to another thread and + * that the handler needs to check it under the idle conns lock. + */ + HA_ATOMIC_OR(&fconn->wait_event.tasklet->state, TASK_F_USR1); + xprt_set_idle(fconn->conn, fconn->conn->xprt, fconn->conn->xprt_ctx); + + /* Ensure session can keep a new idle connection. */ + if (session_check_idle_conn(sess, fconn->conn) != 0) { + CALL_MUX_NO_RET(fconn->conn->mux, destroy(fconn)); + TRACE_DEVEL("outgoing connection killed", FCGI_EV_STRM_END|FCGI_EV_FCONN_ERR); + return 1; + } + + /* At this point, the connection is inserted into + * session list and marked as idle, so it may already + * have been purged from another thread. + */ + TRACE_DEVEL("private connection marked as idle", FCGI_EV_STRM_END, fconn->conn); + return 1; + } + } + else { + if (eb_is_empty(&fconn->streams_by_id)) { + /* If the connection is owned by the session, first remove it + * from its list + */ + if (fconn->conn->owner) { + session_unown_conn(fconn->conn->owner, fconn->conn); + fconn->conn->owner = NULL; + } + + /* mark that the tasklet may lose its context to another thread and + * that the handler needs to check it under the idle conns lock. + */ + HA_ATOMIC_OR(&fconn->wait_event.tasklet->state, TASK_F_USR1); + xprt_set_idle(fconn->conn, fconn->conn->xprt, fconn->conn->xprt_ctx); + + if (!srv_add_to_idle_list(objt_server(fconn->conn->target), fconn->conn, 1)) { + /* The server doesn't want it, let's kill the connection right away */ + CALL_MUX_NO_RET(fconn->conn->mux, destroy(fconn)); + TRACE_DEVEL("outgoing connection killed", FCGI_EV_STRM_END|FCGI_EV_FCONN_ERR); + return 1; + } + /* At this point, the connection has been added to the + * server idle list, so another thread may already have + * hijacked it, so we can't do anything with it. + */ + TRACE_DEVEL("reusable idle connection", FCGI_EV_STRM_END, fconn->conn); + return 1; + } + else if (!ceb_intree(&fconn->conn->hash_node.node) && + fcgi_avail_streams(fconn->conn) > 0 && objt_server(fconn->conn->target) && + !LIST_INLIST(&fconn->conn->sess_el)) { + srv_add_to_avail_list(__objt_server(fconn->conn->target), fconn->conn); + } + } + + return 0; +} + /* * Detach the stream from the connection and possibly release the connection. */ @@ -3858,78 +3958,8 @@ static void fcgi_detach(struct sedesc *sd) /* A stream left, refresh the timeout if necessary. */ fcgi_conn_update_timeout(fconn); - if (!(fconn->flags & (FCGI_CF_EOS|FCGI_CF_ERR_PENDING|FCGI_CF_ERROR)) && - (fconn->state != FCGI_CS_CLOSED) && - (fconn->flags & FCGI_CF_KEEP_CONN)) { - if (fconn->conn->flags & CO_FL_PRIVATE) { - /* Add the connection in the session serverlist, if not already done */ - session_add_conn(sess, fconn->conn); - - if (eb_is_empty(&fconn->streams_by_id)) { - if (!LIST_INLIST(&fconn->conn->sess_el)) { - /* Session insertion above has failed and connection is idle, remove it. */ - CALL_MUX_NO_RET(fconn->conn->mux, destroy(fconn)); - TRACE_DEVEL("outgoing connection killed", FCGI_EV_STRM_END|FCGI_EV_FCONN_ERR); - return; - } - - /* mark that the tasklet may lose its context to another thread and - * that the handler needs to check it under the idle conns lock. - */ - HA_ATOMIC_OR(&fconn->wait_event.tasklet->state, TASK_F_USR1); - xprt_set_idle(fconn->conn, fconn->conn->xprt, fconn->conn->xprt_ctx); - - /* Ensure session can keep a new idle connection. */ - if (session_check_idle_conn(sess, fconn->conn) != 0) { - CALL_MUX_NO_RET(fconn->conn->mux, destroy(fconn)); - TRACE_DEVEL("outgoing connection killed", FCGI_EV_STRM_END|FCGI_EV_FCONN_ERR); - return; - } - - /* At this point, the connection is inserted into - * session list and marked as idle, so it may already - * have been purged from another thread. - */ - TRACE_DEVEL("private connection marked as idle", FCGI_EV_STRM_END, fconn->conn); - return; - } - } - else { - if (eb_is_empty(&fconn->streams_by_id)) { - /* If the connection is owned by the session, first remove it - * from its list - */ - if (fconn->conn->owner) { - session_unown_conn(fconn->conn->owner, fconn->conn); - fconn->conn->owner = NULL; - } - - /* mark that the tasklet may lose its context to another thread and - * that the handler needs to check it under the idle conns lock. - */ - HA_ATOMIC_OR(&fconn->wait_event.tasklet->state, TASK_F_USR1); - xprt_set_idle(fconn->conn, fconn->conn->xprt, fconn->conn->xprt_ctx); - - if (!srv_add_to_idle_list(objt_server(fconn->conn->target), fconn->conn, 1)) { - /* The server doesn't want it, let's kill the connection right away */ - CALL_MUX_NO_RET(fconn->conn->mux, destroy(fconn)); - TRACE_DEVEL("outgoing connection killed", FCGI_EV_STRM_END|FCGI_EV_FCONN_ERR); - return; - } - /* At this point, the connection has been added to the - * server idle list, so another thread may already have - * hijacked it, so we can't do anything with it. - */ - TRACE_DEVEL("reusable idle connection", FCGI_EV_STRM_END, fconn->conn); - return; - } - else if (!ceb_intree(&fconn->conn->hash_node.node) && - fcgi_avail_streams(fconn->conn) > 0 && objt_server(fconn->conn->target) && - !LIST_INLIST(&fconn->conn->sess_el)) { - srv_add_to_avail_list(__objt_server(fconn->conn->target), fconn->conn); - } - } - } + if (fcgi_may_idle_conn(fconn, sess)) + return; TRACE_DEVEL("leaving", FCGI_EV_STRM_END, fconn->conn); } @@ -4072,8 +4102,24 @@ struct task *fcgi_deferred_shut(struct task *t, void *ctx, unsigned int state) /* note: this frees the tasklet as well */ fcgi_strm_destroy(fstrm); t = NULL; - if (fcgi_conn_is_dead(fconn)) + if (fcgi_conn_is_dead(fconn)) { fcgi_release(fconn); + goto end; + } + + /* A stream left, refresh the timeout if necessary. */ + fcgi_conn_update_timeout(fconn); + + /* The stream was blocked in fcgi_detach() which left + * before the connection could be made idle or available + * again, let's do it now. The stream's session cannot be + * used here because it may have been released in the + * mean time. The connection owner is only reliable if the + * connection is still attached to its session, because + * session_free() only resets it in this case. + */ + if (fcgi_may_idle_conn(fconn, (LIST_INLIST(&fconn->conn->sess_el) ? fconn->conn->owner : NULL))) + goto end; } } end: