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 028792e34cd7bf38937d01020fb3ef575957f43c..f3f4ef32cbf34828170c34d12858318a93ec9c90 100644 (file)
--- 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. <sess> 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 <fconn> 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: