From 6042aeb1e8b19da25772a6f77fcce862ff87aaa2 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Tue, 12 Dec 2017 11:01:44 +0100 Subject: [PATCH] BUG/MEDIUM: h2: work around a connection API limitation The connection API permits us to enable or disable receiving on a connection. The underlying FD layer arranges this with the polling and the fd cache. In practice, if receiving was allowed and an end of buffer was reached, the FD is subscribed to the polling. If later we want to process pending data from the buffer, we have to enable receiving again, but since it's already enabled (in polled mode), nothing happens and the pending data remain stuck until a new event happens on the connection to wake the FD up. This is a limitation of the internal connection API which is not very friendly to the new mux architecture. The visible effect is that certain uploads to slow servers experience truncation on timeout on their last blocks because nothing new comes from the connection to wake it up while it's being polled. In order to work around this, there are two solutions : - either cheat on the connection so that conn_update_xprt_polling() always performs a call to fd_may_recv() after fd_want_recv(), that we can trigger from the mux by always calling conn_xprt_stop_recv() before conn_xprt_want_recv(), but that's a bit tricky and may have side effects on other parts (eg: SSL) - or we refrain from receiving in the mux as soon as we're busy on anything else, regardless of whether or not some room is available in the receive buffer. This patch takes the second approach above. This way once we read some data, as soon as we detect that we're stuck, we immediately stop receiving. This ensures the event doesn't go into polled mode for this period and that as soon as we're unstuck we can continue. In fact this guarantees that we can only wait on one side of the mux for a given direction. A future improvement of the connection layer should make it possible to resume processing of an interrupted receive operation. This fix must be backported to 1.8. --- src/mux_h2.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/mux_h2.c b/src/mux_h2.c index fc655db95..570773bfa 100644 --- a/src/mux_h2.c +++ b/src/mux_h2.c @@ -226,8 +226,13 @@ static struct task *h2_timeout_task(struct task *t); * is empty, we must not attempt to receive * - if the demux buf failed to be allocated, we must not try to receive and * we know there is nothing pending - * - if the buffer is not full, we may attempt to receive - * - if no flag indicates a blocking condition, we may attempt to receive + * - if no flag indicates a blocking condition, we may attempt to receive, + * regardless of whether the demux buffer is full or not, so that only + * de demux part decides whether or not to block. This is needed because + * the connection API indeed prevents us from re-enabling receipt that is + * already enabled in a polled state, so we must always immediately stop + * as soon as the demux can't proceed so as never to hit an end of read + * with data pending in the buffers. * - otherwise must may not attempt */ static inline int h2_recv_allowed(const struct h2c *h2c) @@ -239,8 +244,7 @@ static inline int h2_recv_allowed(const struct h2c *h2c) return 0; if (!(h2c->flags & H2_CF_DEM_DALLOC) && - (!(h2c->flags & H2_CF_DEM_DFULL) || - !(h2c->flags & H2_CF_DEM_BLOCK_ANY))) + !(h2c->flags & H2_CF_DEM_BLOCK_ANY)) return 1; return 0;