BUG/MEDIUM: mux-h2: serve all pending buffer allocations in the buffer callback

The demux buffer, the mux buffer and the streams' rxbufs all share the
same buffer_wait entry in the h2c, and the blocking causes are indicated
by the H2_CF_DEM_DALLOC, H2_CF_MUX_MALLOC and H2_CF_DEM_SALLOC flags.
However h2_buf_available() only serves the first cause it manages to
allocate for and returns 1, which dequeues the connection. If another
cause was pending, its flag remains set while the connection is not in
the queue anymore, so nothing will ever retry it:
  - a stale H2_CF_DEM_DALLOC prevents h2_recv() from receiving anymore;
  - a stale H2_CF_DEM_SALLOC blocks the demux forever;
  - a stale H2_CF_MUX_MALLOC prevents h2_send() from calling
    h2_process_mux(), so that window updates, pings and GOAWAY frames are
    not sent anymore, and streams blocked on the connection's flow
    control are never resumed.

This was reproduced with a debug patch faking both a demux buffer and a
mux buffer allocation failure on an H2 connection, then calling
offer_buffers() once 200ms later: a GET for a 200kB object forwarded to
a server stops after exactly 65535 bytes of DATA despite the client's
WINDOW_UPDATE frames, and stays there until the client gives up after
5s, while the whole object is received instantly once both causes are
served.

Let's serve all pending causes in a single call, wake the connection up
if any progress was made, and only accept to leave the queue once none
remain. This needs two simultaneous buffer allocation failures on the
same connection, or a single one combined with the issue fixed by commit
"BUG/MEDIUM: mux-h2: always return already allocated buffers in
h2_get_buf()", so it's rare. The problem exists since the buffer
allocators were merged in 1.9 by commit 44e973f50 ("MEDIUM: h2: use a
single buffer allocator"). The fix may be backported to all supported
versions after the aforementioned one.

diff --git a/src/mux_h2.c b/src/mux_h2.c
index 691dd5afe45fea3465dbc907767a05be5ad064c3..70426c52987c0b8fa0d4ff6c37b3138f3c6ee764 100644 (file)
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -1257,14 +1257,18 @@ static inline int h2_frt_has_too_many_sc(const struct h2c *h2c)
 }
 
 /* Tries to grab a buffer and to re-enable processing on mux <target>. The h2c
- * flags are used to figure what buffer was requested. It returns 1 if the
- * allocation succeeds, in which case the connection is woken up, or 0 if it's
- * impossible to wake up and we prefer to be woken up later.
+ * flags are used to figure what buffers were requested, since the same
+ * buffer_wait is shared by all of them, and all of them are served at once.
+ * It returns 1 once all requested allocations succeeded, in which case the
+ * connection is dequeued. Otherwise it returns 0 so that the connection stays
+ * in the queue and gets called again for the remaining ones. In any case the
+ * connection is woken up if some progress was made.
  */
 static int h2_buf_available(void *target)
 {
        struct h2c *h2c = target;
        struct h2s *h2s;
+       int progress = 0;
 
        /* Note: we're still in the buffer wait queue when called, so we must
         * pass DB_F_NOQUEUE to b_alloc() otherwise our own presence in the
@@ -1273,7 +1277,7 @@ static int h2_buf_available(void *target)
        if ((h2c->flags & H2_CF_DEM_DALLOC) && b_alloc(&h2c->dbuf, DB_MUX_RX | DB_F_NOQUEUE)) {
                h2c->flags &= ~H2_CF_DEM_DALLOC;
                h2c_restart_reading(h2c, 1);
-               return 1;
+               progress = 1;
        }
 
        if ((h2c->flags & H2_CF_MUX_MALLOC) && b_alloc(br_tail(h2c->mbuf), DB_MUX_TX | DB_F_NOQUEUE)) {
@@ -1285,7 +1289,7 @@ static int h2_buf_available(void *target)
                }
                /* streams blocked on the mbuf are resumed by h2_send() */
                tasklet_wakeup(h2c->wait_event.tasklet);
-               return 1;
+               progress = 1;
        }
 
        if ((h2c->flags & H2_CF_DEM_SALLOC) &&
@@ -1295,11 +1299,15 @@ static int h2_buf_available(void *target)
                if (b_alloc(_h2s_rxbuf_tail(h2s), DB_SE_RX | DB_F_NOQUEUE)) {
                        h2c->flags &= ~H2_CF_DEM_SALLOC;
                        h2c_restart_reading(h2c, 1);
-                       return 1;
+                       progress = 1;
                }
        }
 
-       return 0;
+       /* stay in the queue as long as some allocations are still pending,
+        * otherwise nobody would ever retry them.
+        */
+       return progress &&
+               !(h2c->flags & (H2_CF_DEM_DALLOC | H2_CF_MUX_MALLOC | H2_CF_DEM_SALLOC));
 }
 
 /* note that bptr may not be NULL here */