From 0943757a2144761c60e416b5ed07baa76934f5a4 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Wed, 21 May 2014 16:58:17 +0200 Subject: [PATCH] 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. --- src/session.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/session.c b/src/session.c index 727182bb37..d2c2850eab 100644 --- 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. */ -- 2.39.5