BUG/MEDIUM: stream: fix half-closed timeout handling

client-fin and server-fin are bogus. They are applied on the write
side after a SHUTR was seen. The immediate effect is that sometimes
if a SHUTR was seen after a SHUTW on the same side, the timeout is
enabled again regardless of the fact that the output is already
closed. This results in the timeout event not to be processed and
a busy poll loop to happen until another timeout on the stream gets
rid of it. Note that haproxy continues its job during this, it's just
that it eats all the CPU trying to handle an event that it ignores.

An reproducible case consists in having a client stop reading data from
a server to ensure data remain in the response buffer, then the client
sends a shutdown(write). If abortonclose is enabled on haproxy, the
shutdown is passed to the server side and the server responds with a
SHUTR that cannot immediately be forwarded to the client since the
buffer is full. During this time the event is ignored and the task is
woken again in loops.

It is worth noting that the timeout handling since 1.5 is a bit fragile
and that it might be possible that other similar conditions still exist,
so the timeout handling should be audited regarding this issue.

Many thanks to BaiYang for providing detailed information showing the
problem in action.

This bug also affects 1.5 thus the fix must be backported.

diff --git a/src/stream.c b/src/stream.c
index 13108968396cde3ab69b37074fc61e8abf6fcc73..e98ba9f2d258c0ab8c08fece03b686ad386170c1 100644 (file)
--- a/src/stream.c
+++ b/src/stream.c
@@ -2091,10 +2091,6 @@ struct task *process_stream(struct task *t)
        if (unlikely((req->flags & (CF_SHUTW|CF_SHUTW_NOW|CF_AUTO_CLOSE|CF_SHUTR)) ==
                     (CF_AUTO_CLOSE|CF_SHUTR))) {
                channel_shutw_now(req);
-               if (tick_isset(sess->fe->timeout.clientfin)) {
-                       res->wto = sess->fe->timeout.clientfin;
-                       res->wex = tick_add(now_ms, res->wto);
-               }
        }
 
        /* shutdown(write) pending */
@@ -2119,10 +2115,6 @@ struct task *process_stream(struct task *t)
                if (si_f->flags & SI_FL_NOHALF)
                        si_f->flags |= SI_FL_NOLINGER;
                si_shutr(si_f);
-               if (tick_isset(sess->fe->timeout.clientfin)) {
-                       res->wto = sess->fe->timeout.clientfin;
-                       res->wex = tick_add(now_ms, res->wto);
-               }
        }
 
        /* it's possible that an upper layer has requested a connection setup or abort.
@@ -2284,10 +2276,6 @@ struct task *process_stream(struct task *t)
        if (unlikely((res->flags & (CF_SHUTW|CF_SHUTW_NOW|CF_AUTO_CLOSE|CF_SHUTR)) ==
                     (CF_AUTO_CLOSE|CF_SHUTR))) {
                channel_shutw_now(res);
-               if (tick_isset(s->be->timeout.serverfin)) {
-                       req->wto = s->be->timeout.serverfin;
-                       req->wex = tick_add(now_ms, req->wto);
-               }
        }
 
        /* shutdown(write) pending */
@@ -2310,10 +2298,6 @@ struct task *process_stream(struct task *t)
                if (si_b->flags & SI_FL_NOHALF)
                        si_b->flags |= SI_FL_NOLINGER;
                si_shutr(si_b);
-               if (tick_isset(s->be->timeout.serverfin)) {
-                       req->wto = s->be->timeout.serverfin;
-                       req->wex = tick_add(now_ms, req->wto);
-               }
        }
 
        if (si_f->state == SI_ST_DIS || si_b->state == SI_ST_DIS)