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

h2_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, so a connection detached with data still queued in
the mux buffer is parked without any timeout and nothing drains it. The
timeout used to be refreshed for idle-ping only, which left every other
case aside.

The refresh is now performed right after h2s_destroy(), before the
connection may be given away, and the redundant 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.
h2c_update_timeout() already returns early when there is no task, so no
guard is needed at the call site.

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
h2_release() clears its task context. This only reorders statements:
h2c_update_timeout() touches neither the stream map nor the flags and
states h2c_is_dead() looks at, so the predicate gives the same result at
both places. Since a dead connection now returns early, the
"!h2c_is_dead(h2c)" test that guarded the idle-list branches, and the
h2c_is_dead() check that closed the function, are both redundant and are
removed. Commit 59c599f3f ("BUG/MEDIUM: mux-h2: make sure not to move a
dead connection to idle") is thus enforced structurally rather than by a
duplicated test.

This could be backported as far as 2.6.

diff --git a/src/mux_h2.c b/src/mux_h2.c
index e939ced0d43b5eacaa75543f2a4716eb2f40822d..8293ea22726ece259856553c9b92549e489800b8 100644 (file)
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -5856,14 +5856,21 @@ static void h2_detach(struct sedesc *sd)
 
        h2s_destroy(h2s);
 
-       if (h2c->flags & H2_CF_IS_BACK && !h2c_is_dead(h2c)) {
-               if (!(h2c->flags & (H2_CF_RCVD_SHUT|H2_CF_ERR_PENDING|H2_CF_ERROR))) {
-                       /* Ensure idle-ping is activated before going to idle. */
-                       if (eb_is_empty(&h2c->streams_by_id) &&
-                           tick_isset(conn_idle_ping(h2c->conn))) {
-                               h2c_update_timeout(h2c);
-                       }
+       /* Don't close unless the last stream is gone and either the
+        * connection is in error, or it reached the ID already specified in
+        * a GOAWAY frame received or sent (h2c_is_dead() documents this).
+        */
+       if (h2c_is_dead(h2c)) {
+               TRACE_DEVEL("leaving and killing dead connection", H2_EV_STRM_END, h2c->conn);
+               h2_release(h2c);
+               return;
+       }
+
+       /* A stream left, refresh the timeout if necessary. */
+       h2c_update_timeout(h2c);
 
+       if (h2c->flags & H2_CF_IS_BACK) {
+               if (!(h2c->flags & (H2_CF_RCVD_SHUT|H2_CF_ERR_PENDING|H2_CF_ERROR))) {
                        if (h2c->conn->flags & CO_FL_PRIVATE) {
                                /* Add the connection in the session server list, if not already done */
                                session_add_conn(sess, h2c->conn);
@@ -5936,22 +5943,7 @@ static void h2_detach(struct sedesc *sd)
                }
        }
 
-       /* We don't want to close right now unless we're removing the
-        * last stream, and either the connection is in error, or it
-        * reached the ID already specified in a GOAWAY frame received
-        * or sent (as seen by last_sid >= 0).
-        */
-       if (h2c_is_dead(h2c)) {
-               /* no more stream will come, kill it now */
-               TRACE_DEVEL("leaving and killing dead connection", H2_EV_STRM_END, h2c->conn);
-               h2_release(h2c);
-       }
-       else if (h2c->task) {
-               h2c_update_timeout(h2c);
-               TRACE_DEVEL("leaving, refreshing connection's timeout", H2_EV_STRM_END, h2c->conn);
-       }
-       else
-               TRACE_DEVEL("leaving", H2_EV_STRM_END, h2c->conn);
+       TRACE_DEVEL("leaving", H2_EV_STRM_END, h2c->conn);
 }
 
 /* Performs a synchronous or asynchronous shutr(). */