BUG/MEDIUM: session: don't clear CF_READ_NOEXP if analysers are not called

As more or less suspected, commit b1982e2 ("BUG/MEDIUM: http/session:
disable client-side expiration only after body") was hazardous. It
introduced a regression causing client side timeout to expire during
connection retries if it's lower than the time needed to cover the
amount of retries, so clients get a 408 when the connection to the
server fails to establish fast enough.

The reason is that the CF_READ_NOEXP flag is set after the MSG_DONE state
is reached, which protects the timeout from being re-armed, then during
the retries, process_session() clears the flag without calling the analyser
(since there's no activity for it), so the timeouts are rearmed.

Ideally, these one-shot flags should be per-analyser, and the analyser
which sets them would be responsible for clearing them, or they would
automatically be cleared when switching to another analyser. Unfortunately
this is not really possible currently.

What can be done however is to only clear them in the following situations :
  - we're going to call analysers
  - analysers have all been unsubscribed

This method seems reliable enough and approaches the ideal case well enough.

No backport is needed, this bug was introduced in 1.5-dev25.

diff --git a/src/session.c b/src/session.c
index 727182bb37fc5d017eb63ecc0047580f220f39b2..d2c2850eabb490f150a9af19cc1e86f3e1cf2f26 100644 (file)
--- a/src/session.c
+++ b/src/session.c
@@ -1619,6 +1619,7 @@ struct task *process_session(struct task *t)
        unsigned int rq_prod_last, rq_cons_last;
        unsigned int rp_cons_last, rp_prod_last;
        unsigned int req_ana_back;
+       unsigned int rq_oneshot, rp_oneshot;
 
        //DPRINTF(stderr, "%s:%d: cs=%d ss=%d(%d) rqf=0x%08x rpf=0x%08x\n", __FUNCTION__, __LINE__,
        //        s->si[0].state, s->si[1].state, s->si[1].err_type, s->req->flags, s->rep->flags);
@@ -1626,9 +1627,13 @@ struct task *process_session(struct task *t)
        /* this data may be no longer valid, clear it */
        memset(&s->txn.auth, 0, sizeof(s->txn.auth));
 
-       /* This flag must explicitly be set every time */
-       s->req->flags &= ~(CF_READ_NOEXP|CF_WAKE_WRITE);
-       s->rep->flags &= ~(CF_READ_NOEXP|CF_WAKE_WRITE);
+       /* These flags must explicitly be set every time by the analysers who
+        * need them, but we won't always call them (eg: during a connection
+        * retry). So we need to keep them and only clear them if we're sure
+        * to call the analysers.
+        */
+       rq_oneshot = s->req->flags & (CF_READ_NOEXP | CF_WAKE_WRITE);
+       rp_oneshot = s->rep->flags & (CF_READ_NOEXP | CF_WAKE_WRITE);
 
        /* Keep a copy of req/rep flags so that we can detect shutdowns */
        rqf_last = s->req->flags & ~CF_MASK_ANALYSER;
@@ -1809,6 +1814,8 @@ struct task *process_session(struct task *t)
            s->si[1].state != rq_cons_last) {
                unsigned int flags = s->req->flags;
 
+               s->req->flags &= ~rq_oneshot;
+               rq_oneshot = 0;
                if (s->req->prod->state >= SI_ST_EST) {
                        int max_loops = global.tune.maxpollevents;
                        unsigned int ana_list;
@@ -1962,11 +1969,13 @@ struct task *process_session(struct task *t)
        /* Analyse response */
 
        if (((s->rep->flags & ~rpf_last) & CF_MASK_ANALYSER) ||
-                (s->rep->flags ^ rpf_last) & CF_MASK_STATIC ||
-                s->si[0].state != rp_cons_last ||
-                s->si[1].state != rp_prod_last) {
+           ((s->rep->flags ^ rpf_last) & CF_MASK_STATIC) ||
+           s->si[0].state != rp_cons_last ||
+           s->si[1].state != rp_prod_last) {
                unsigned int flags = s->rep->flags;
 
+               s->rep->flags &= ~rp_oneshot;
+               rp_oneshot = 0;
                if ((s->rep->flags & CF_MASK_ANALYSER) &&
                    (s->rep->analysers & AN_REQ_WAIT_HTTP)) {
                        /* Due to HTTP pipelining, the HTTP request analyser might be waiting
@@ -2160,6 +2169,9 @@ struct task *process_session(struct task *t)
                channel_auto_close(s->req);
                buffer_flush(s->req->buf);
 
+               s->req->flags &= ~rq_oneshot;
+               rq_oneshot = 0;
+
                /* We'll let data flow between the producer (if still connected)
                 * to the consumer (which might possibly not be connected yet).
                 */
@@ -2315,6 +2327,9 @@ struct task *process_session(struct task *t)
                channel_auto_close(s->rep);
                buffer_flush(s->rep->buf);
 
+               s->rep->flags &= ~rp_oneshot;
+               rp_oneshot = 0;
+
                /* We'll let data flow between the producer (if still connected)
                 * to the consumer.
                 */