BUG/MINOR: http: Fix conditions to clean up a txn and to handle the next request

To finish a HTTP transaction and to start the new one, we check, among other
things, that there is enough space in the reponse buffer to eventually inject a
message during the parsing of the next request. Because these messages can reach
the maximum buffers size, it is mandatory to have an empty response
buffer. Remaining input data are trimmed during the txn cleanup (in
http_reset_txn), so we just need to check that the output data were flushed.

The current implementation depends on channel_congested, which does check the
reserved area is available. That's not of course good enough. There are other
tests on the reponse buffer is http_wait_for_request. But conditions to move on
are almost the same. So, we can imagine some scenarii where some output data
remaining in the reponse buffer during the request parsing prevent any messages
injection.

To fix this bug, we just wait that output data were flushed before cleaning up
the HTTP txn (ie. s->res.buf->o == 0). In addition, in http_reset_txn we realign
the response buffer (note the buffer is empty at this step).

Thanks to this changes, there is no more need to set CF_EXPECT_MORE on the
response channel in http_end_txn_clean_session. And more important, there is no
more need to check the response buffer state in http_wait_for_request. This
remove a workaround on response analysers to handle HTTP pipelining.

This patch can be backported in 1.7, 1.6 and 1.5.

diff --git a/src/proto_http.c b/src/proto_http.c
index 56480da657ab0e0f43b72790a9285528b58306fe..3b6f0afa4d95bcbc044f82e3d480db907e179c3f 100644 (file)
--- a/src/proto_http.c
+++ b/src/proto_http.c
@@ -2649,29 +2649,6 @@ int http_wait_for_request(struct stream *s, struct channel *req, int an_bit)
                                buffer_slow_realign(req->buf);
                }
 
-               /* Note that we have the same problem with the response ; we
-                * may want to send a redirect, error or anything which requires
-                * some spare space. So we'll ensure that we have at least
-                * maxrewrite bytes available in the response buffer before
-                * processing that one. This will only affect pipelined
-                * keep-alive requests.
-                */
-               if ((txn->flags & TX_NOT_FIRST) &&
-                   unlikely(!channel_is_rewritable(&s->res) ||
-                            bi_end(s->res.buf) < b_ptr(s->res.buf, txn->rsp.next) ||
-                            bi_end(s->res.buf) > s->res.buf->data + s->res.buf->size - global.tune.maxrewrite)) {
-                       if (s->res.buf->o) {
-                               if (s->res.flags & (CF_SHUTW|CF_SHUTW_NOW|CF_WRITE_ERROR|CF_WRITE_TIMEOUT))
-                                       goto failed_keep_alive;
-                               /* don't let a connection request be initiated */
-                               channel_dont_connect(req);
-                               s->res.flags &= ~CF_EXPECT_MORE; /* speed up sending a previous response */
-                               s->res.flags |= CF_WAKE_WRITE;
-                               s->res.analysers |= an_bit; /* wake us up once it changes */
-                               return 0;
-                       }
-               }
-
                if (likely(msg->next < req->buf->i)) /* some unparsed data are available */
                        http_msg_analyzer(msg, &txn->hdr_idx);
        }
@@ -5301,20 +5278,6 @@ void http_end_txn_clean_session(struct stream *s)
                s->res.flags |= CF_NEVER_WAIT;
        }
 
-       /* if the request buffer is not empty, it means we're
-        * about to process another request, so send pending
-        * data with MSG_MORE to merge TCP packets when possible.
-        * Just don't do this if the buffer is close to be full,
-        * because the request will wait for it to flush a little
-        * bit before proceeding.
-        */
-       if (s->req.buf->i) {
-               if (s->res.buf->o &&
-                   !buffer_full(s->res.buf, global.tune.maxrewrite) &&
-                   bi_end(s->res.buf) <= s->res.buf->data + s->res.buf->size - global.tune.maxrewrite)
-                       s->res.flags |= CF_EXPECT_MORE;
-       }
-
        /* we're removing the analysers, we MUST re-enable events detection.
         * We don't enable close on the response channel since it's either
         * already closed, or in keep-alive with an idle connection handler.
@@ -5688,13 +5651,13 @@ int http_resync_states(struct stream *s)
                 * possibly killing the server connection and reinitialize
                 * a fresh-new transaction, but only once we're sure there's
                 * enough room in the request and response buffer to process
-                * another request. The request buffer must not hold any
-                * pending output data and the request buffer must not have
-                * output data occupying the reserve.
+                * another request. They must not hold any pending output data
+                * and the response buffer must realigned
+                * (realign is done is http_end_txn_clean_session).
                 */
                if (s->req.buf->o)
                        s->req.flags |= CF_WAKE_WRITE;
-               else if (channel_congested(&s->res))
+               else if (s->res.buf->o)
                        s->res.flags |= CF_WAKE_WRITE;
                else {
                        s->req.analysers = AN_REQ_FLT_END;
@@ -9021,6 +8984,9 @@ void http_reset_txn(struct stream *s)
        if (unlikely(s->res.buf->i))
                s->res.buf->i = 0;
 
+       /* Now we can realign the response buffer */
+       buffer_realign(s->res.buf);
+
        s->req.rto = strm_fe(s)->timeout.client;
        s->req.wto = TICK_ETERNITY;
 

diff --git a/src/stream.c b/src/stream.c
index 78cc7ff5cff1857014f59bb70b875c73c200c2ee..bc8b3af138a1b0ff60aa2a668b136d97c6c76de3 100644 (file)
--- a/src/stream.c
+++ b/src/stream.c
@@ -1834,18 +1834,6 @@ struct task *process_stream(struct task *t)
                 s->pending_events & TASK_WOKEN_MSG) {
                unsigned int flags = res->flags;
 
-               if ((res->flags & CF_MASK_ANALYSER) &&
-                   (res->analysers & AN_REQ_ALL)) {
-                       /* Due to HTTP pipelining, the HTTP request analyser might be waiting
-                        * for some free space in the response buffer, so we might need to call
-                        * it when something changes in the response buffer, but still we pass
-                        * it the request buffer. Note that the SI state might very well still
-                        * be zero due to us returning a flow of redirects!
-                        */
-                       res->analysers &= ~AN_REQ_ALL;
-                       req->flags |= CF_WAKE_ONCE;
-               }
-
                if (si_b->state >= SI_ST_EST) {
                        int max_loops = global.tune.maxpollevents;
                        unsigned int ana_list;