MINOR: mux-fcgi: check for a dead connection before refreshing the timeout

fcgi_detach() refreshed the connection timeout right after the stream was
destroyed, then evaluated fcgi_conn_is_dead() only at the end of the
function, on the paths that did not already return. So a connection about to
be released was first armed and requeued via task_queue(), then
fcgi_release() cleared its task context and woke it up again for nothing.

Let's evaluate fcgi_conn_is_dead() first, right after the stream is
destroyed, and return early when the connection must be released. The check
that closed the function is then redundant and is removed, and the timeout
is only refreshed for connections that survive.

This only reorders statements: fcgi_conn_update_timeout() writes the task's
expiration date, the FCGI_CF_IDL_PING-like state and the timer queue, none
of which fcgi_conn_is_dead() reads. It also matches the structure mux-h2
just got. The move is safe with regards to the idle lists, since the
branches below only set conn->owner, the session and idle list entries and
the transport idle state, which the predicate does not look at either.

Note that this was only correct once the previous commit removed the
conn->owner test from fcgi_conn_is_dead(): the non-private branch below
clears conn->owner before parking the connection, so evaluating the
predicate first would otherwise report every connection as dead.

This relates to GitHub issue #3558.

diff --git a/src/mux_fcgi.c b/src/mux_fcgi.c
index d83858b2cb3289e57e63ccb5ceb67aa645123d6c..c471e2a769ee7aae4ea242300538ad5ebb2c5d41 100644 (file)
--- a/src/mux_fcgi.c
+++ b/src/mux_fcgi.c
@@ -3845,10 +3845,17 @@ static void fcgi_detach(struct sedesc *sd)
 
        fcgi_strm_destroy(fstrm);
 
-       /* the stream connector left, so the connection may expire again: arm
-        * the timeout before the connection may be moved to an idle list below,
-        * otherwise it would stay there without any.
+       /* Don't close unless the last stream is gone and either the
+        * connection is in error, or it may not be kept alive
+        * (fcgi_conn_is_dead() documents this).
         */
+       if (fcgi_conn_is_dead(fconn)) {
+               TRACE_DEVEL("leaving, killing dead connection", FCGI_EV_STRM_END, fconn->conn);
+               fcgi_release(fconn);
+               return;
+       }
+
+       /* 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)) &&
@@ -3923,16 +3930,7 @@ static void fcgi_detach(struct sedesc *sd)
                }
        }
 
-       /* We don't want to close right now unless we're removing the last
-        * stream and the connection is in error.
-        */
-       if (fcgi_conn_is_dead(fconn)) {
-               /* no more stream will come, kill it now */
-               TRACE_DEVEL("leaving, killing dead connection", FCGI_EV_STRM_END, fconn->conn);
-               fcgi_release(fconn);
-       }
-       else
-               TRACE_DEVEL("leaving", FCGI_EV_STRM_END, fconn->conn);
+       TRACE_DEVEL("leaving", FCGI_EV_STRM_END, fconn->conn);
 }