BUG/MEDIUM: mux-spop: refresh the connection timeout before parking it idle

spop_detach() refreshes the connection timeout in two places only: in the
early return taken when the stream is blocked, and at the very end of the
function. The branches that move the connection to an idle list (the
session's private list or the server's idle pool) return before reaching
the tail. Since the timeout is disabled while streams are attached, a
connection detached with data still queued in the mux buffer is thus parked
without any timeout and nothing drains it. It is only released by the purge
of the idle connections.

The refresh is now performed right after spop_strm_destroy(), before the
connection may be given away, and the call at the end of the function is
dropped: everything between the two points only manipulates session and
idle lists, so it recomputed the very same expiration date.
spop_conn_update_timeout() already returns early when there is no task, so
no guard is needed at the call site. Note that an idle connection without
pending data now also gets the default timeout when it is parked, as it is
already the case each time it is processed by spop_process().

The dead connection check is moved ahead of the refresh, so that a
connection about to be released is not armed and requeued just before
spop_release() clears its task context. This only reorders statements:
spop_conn_update_timeout() touches neither the stream tree nor the flags
and states spop_conn_is_dead() looks at. And since the previous commit
removed the conn->owner test from spop_conn_is_dead(), clearing conn->owner
in the non-private branch below has no effect on the predicate.

This is the same fix as the one applied on mux-h2 by commit 5e7bb2063
("BUG/MEDIUM: mux-h2: refresh the connection timeout before parking it
idle") and on mux-fcgi by commits a167d39a5 ("BUG/MEDIUM: mux-fcgi: arm the
connection timeout only when data remains") and b77b044d3 ("MINOR:
mux-fcgi: check for a dead connection before refreshing the timeout").

This should be backported to 3.1 with the previous commit.

diff --git a/src/mux_spop.c b/src/mux_spop.c
index 7d408708784797243988962a863d135f4b8f2081..a742c930ea0552a085c50452f7e6d9c31655e872 100644 (file)
--- a/src/mux_spop.c
+++ b/src/mux_spop.c
@@ -3011,6 +3011,19 @@ static void spop_detach(struct sedesc *sd)
 
        spop_strm_destroy(spop_strm);
 
+       /* Don't close unless the last stream is gone and either the
+        * connection is in error, or it was closed (spop_conn_is_dead()
+        * documents this).
+        */
+       if (spop_conn_is_dead(spop_conn)) {
+               TRACE_DEVEL("leaving, killing dead connection", SPOP_EV_STRM_END, spop_conn->conn);
+               spop_release(spop_conn);
+               return;
+       }
+
+       /* A stream left, refresh the timeout if necessary. */
+       spop_conn_update_timeout(spop_conn);
+
        if (!(spop_conn->flags & (SPOP_CF_RCVD_SHUT|SPOP_CF_ERR_PENDING|SPOP_CF_ERROR))) {
                if (spop_conn->conn->flags & CO_FL_PRIVATE) {
                        /* Add the connection in the session server list, if not already done */
@@ -3082,20 +3095,7 @@ static void spop_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 (spop_conn_is_dead(spop_conn)) {
-               /* no more stream will come, kill it now */
-               TRACE_DEVEL("leaving, killing dead connection", SPOP_EV_STRM_END, spop_conn->conn);
-               spop_release(spop_conn);
-       }
-       else if (spop_conn->task) {
-               spop_conn_update_timeout(spop_conn);
-               TRACE_DEVEL("leaving, refreshing connection's timeout", SPOP_EV_STRM_END, spop_conn->conn);
-       }
-       else
-               TRACE_DEVEL("leaving", SPOP_EV_STRM_END, spop_conn->conn);
+       TRACE_DEVEL("leaving", SPOP_EV_STRM_END, spop_conn->conn);
 }
 
 /* Performs a synchronous or asynchronous shutr(). */