[MAJOR] session: fix the order by which the analysers are run
With the introduction of keep-alive, we have created situations
where an analyser can add other analysers to the current list,
which are behind it, which have already been processed once, and
which are needed immediately because without them there will be
no more I/O activity. This is typically the case for enabling
reading of a new request after preparing for a new request.
Instead of creating specific cases for some analysers (there was
already one such before), we now use a little bit of algorithmics
to create an ordered bit chain supporting priorities and fast
operations.
Another advantage of this new construction is that it's not a
real loop anymore, so if an analyser is unknown, it will not
loop but just ignore it.
Note that it is easy to skip multiple analysers at once now in
order to speed up the checking a bit. Some test code has shown
a minor gain though.
This change has been carefully re-read and has no direct reason
of causing a regression. However it has been tagged "major"
because the fact that it runs the analysers correctly might
trigger an old sleeping bug somewhere in one of the analysers.
diff --git a/src/session.c b/src/session.c
index a22a6f3..78f305f 100644
--- a/src/session.c
+++ b/src/session.c
@@ -624,6 +624,18 @@
return 0;
}
+/* This macro is very specific to the function below. See the comments in
+ * process_session() below to understand the logic and the tests.
+ */
+#define UPDATE_ANALYSERS(real, list, back, flag) { \
+ list = (((list) & ~(flag)) | ~(back)) & (real); \
+ back = real; \
+ if (!(list)) \
+ break; \
+ if (((list) ^ ((list) & ((list) - 1))) < (flag)) \
+ continue; \
+}
+
/* Processes the client, server, request and response jobs of a session task,
* then puts it back to the wait queue in a clean state, or cleans up its
* resources if it must be deleted. Returns in <next> the date the task wants
@@ -788,7 +800,8 @@
unsigned int flags = s->req->flags;
if (s->req->prod->state >= SI_ST_EST) {
- unsigned int last_ana = 0;
+ unsigned int ana_list;
+ unsigned int ana_back;
/* it's up to the analysers to stop new connections */
buffer_auto_connect(s->req);
@@ -803,77 +816,98 @@
* kill the session. We loop at least once through each
* analyser, and we may loop again if other analysers
* are added in the middle.
+ *
+ * We build a list of analysers to run. We evaluate all
+ * of these analysers in the order of the lower bit to
+ * the higher bit. This ordering is very important.
+ * An analyser will often add/remove other analysers,
+ * including itself. Any changes to itself have no effect
+ * on the loop. If it removes any other analysers, we
+ * want those analysers not to be called anymore during
+ * this loop. If it adds an analyser that is located
+ * after itself, we want it to be scheduled for being
+ * processed during the loop. If it adds an analyser
+ * which is located before it, we want it to switch to
+ * it immediately, even if it has already been called
+ * once but removed since.
+ *
+ * In order to achieve this, we compare the analyser
+ * list after the call with a copy of it before the
+ * call. The work list is fed with analyser bits that
+ * appeared during the call. Then we compare previous
+ * work list with the new one, and check the bits that
+ * appeared. If the lowest of these bits is lower than
+ * the current bit, it means we have enabled a previous
+ * analyser and must immediately loop again.
*/
- while (s->req->analysers & ~last_ana) {
- last_ana = s->req->analysers;
+
+ ana_list = ana_back = s->req->analysers;
+ do {
+ if (!ana_list)
+ break;
+
+ /* Warning! ensure that analysers are always placed in ascending order! */
- if (s->req->analysers & AN_REQ_INSPECT) {
- last_ana |= AN_REQ_INSPECT;
+ if (ana_list & AN_REQ_INSPECT) {
if (!tcp_inspect_request(s, s->req, AN_REQ_INSPECT))
break;
+ UPDATE_ANALYSERS(s->req->analysers, ana_list, ana_back, AN_REQ_INSPECT);
}
- if (s->req->analysers & AN_REQ_WAIT_HTTP) {
- last_ana |= AN_REQ_WAIT_HTTP;
+ if (ana_list & AN_REQ_WAIT_HTTP) {
if (!http_wait_for_request(s, s->req, AN_REQ_WAIT_HTTP))
break;
+ UPDATE_ANALYSERS(s->req->analysers, ana_list, ana_back, AN_REQ_WAIT_HTTP);
}
- if (s->req->analysers & AN_REQ_HTTP_PROCESS_FE) {
- last_ana |= AN_REQ_HTTP_PROCESS_FE;
+ if (ana_list & AN_REQ_HTTP_PROCESS_FE) {
if (!http_process_req_common(s, s->req, AN_REQ_HTTP_PROCESS_FE, s->fe))
break;
+ UPDATE_ANALYSERS(s->req->analysers, ana_list, ana_back, AN_REQ_HTTP_PROCESS_FE);
}
- if (s->req->analysers & AN_REQ_SWITCHING_RULES) {
- last_ana |= AN_REQ_SWITCHING_RULES;
+ if (ana_list & AN_REQ_SWITCHING_RULES) {
if (!process_switching_rules(s, s->req, AN_REQ_SWITCHING_RULES))
break;
- /* FIXME: we mait switch from TCP to HTTP and want to
- * immediately loop back to the top. This is a dirty way
- * of doing it, and we should find a cleaner method relying
- * on a circular list of function pointers.
- */
- if ((s->req->analysers & ~last_ana) & AN_REQ_WAIT_HTTP)
- continue;
+ UPDATE_ANALYSERS(s->req->analysers, ana_list, ana_back, AN_REQ_SWITCHING_RULES);
}
- if (s->req->analysers & AN_REQ_HTTP_PROCESS_BE) {
- last_ana |= AN_REQ_HTTP_PROCESS_BE;
+ if (ana_list & AN_REQ_HTTP_PROCESS_BE) {
if (!http_process_req_common(s, s->req, AN_REQ_HTTP_PROCESS_BE, s->be))
break;
+ UPDATE_ANALYSERS(s->req->analysers, ana_list, ana_back, AN_REQ_HTTP_PROCESS_BE);
}
- if (s->req->analysers & AN_REQ_HTTP_TARPIT) {
- last_ana |= AN_REQ_HTTP_TARPIT;
+ if (ana_list & AN_REQ_HTTP_TARPIT) {
if (!http_process_tarpit(s, s->req, AN_REQ_HTTP_TARPIT))
break;
+ UPDATE_ANALYSERS(s->req->analysers, ana_list, ana_back, AN_REQ_HTTP_TARPIT);
}
- if (s->req->analysers & AN_REQ_HTTP_INNER) {
- last_ana |= AN_REQ_HTTP_INNER;
+ if (ana_list & AN_REQ_HTTP_INNER) {
if (!http_process_request(s, s->req, AN_REQ_HTTP_INNER))
break;
+ UPDATE_ANALYSERS(s->req->analysers, ana_list, ana_back, AN_REQ_HTTP_INNER);
}
- if (s->req->analysers & AN_REQ_HTTP_BODY) {
- last_ana |= AN_REQ_HTTP_BODY;
+ if (ana_list & AN_REQ_HTTP_BODY) {
if (!http_process_request_body(s, s->req, AN_REQ_HTTP_BODY))
break;
+ UPDATE_ANALYSERS(s->req->analysers, ana_list, ana_back, AN_REQ_HTTP_BODY);
}
- if (s->req->analysers & AN_REQ_PRST_RDP_COOKIE) {
- last_ana |= AN_REQ_PRST_RDP_COOKIE;
+ if (ana_list & AN_REQ_PRST_RDP_COOKIE) {
if (!tcp_persist_rdp_cookie(s, s->req, AN_REQ_PRST_RDP_COOKIE))
break;
+ UPDATE_ANALYSERS(s->req->analysers, ana_list, ana_back, AN_REQ_PRST_RDP_COOKIE);
}
- if (s->req->analysers & AN_REQ_HTTP_XFER_BODY) {
- last_ana |= AN_REQ_HTTP_XFER_BODY;
+ if (ana_list & AN_REQ_HTTP_XFER_BODY) {
if (!http_request_forward_body(s, s->req, AN_REQ_HTTP_XFER_BODY))
break;
+ UPDATE_ANALYSERS(s->req->analysers, ana_list, ana_back, AN_REQ_HTTP_XFER_BODY);
}
- }
+ } while (0);
}
if ((s->req->flags ^ flags) & BF_MASK_STATIC) {
@@ -906,7 +940,8 @@
unsigned int flags = s->rep->flags;
if (s->rep->prod->state >= SI_ST_EST) {
- unsigned int last_ana = 0;
+ unsigned int ana_list;
+ unsigned int ana_back;
/* it's up to the analysers to reset auto_close */
buffer_auto_close(s->rep);
@@ -921,34 +956,32 @@
* analyser, and we may loop again if other analysers
* are added in the middle.
*/
- while (s->rep->analysers & ~last_ana) {
- last_ana = s->rep->analysers;
+
+ ana_list = ana_back = s->rep->analysers;
+ do {
+ if (!ana_list)
+ break;
- if (s->rep->analysers & AN_RES_WAIT_HTTP) {
- last_ana |= AN_RES_WAIT_HTTP;
+ /* Warning! ensure that analysers are always placed in ascending order! */
+
+ if (ana_list & AN_RES_WAIT_HTTP) {
if (!http_wait_for_response(s, s->rep, AN_RES_WAIT_HTTP))
break;
+ UPDATE_ANALYSERS(s->rep->analysers, ana_list, ana_back, AN_RES_WAIT_HTTP);
}
- if (s->rep->analysers & AN_RES_HTTP_PROCESS_BE) {
- last_ana |= AN_RES_HTTP_PROCESS_BE;
+ if (ana_list & AN_RES_HTTP_PROCESS_BE) {
if (!http_process_res_common(s, s->rep, AN_RES_HTTP_PROCESS_BE, s->be))
break;
- /* FIXME: we may wait for a second response in case of a status 1xx
- * and want to immediately loop back to the top. This is a dirty way
- * of doing it, and we should find a cleaner method relying on a
- * circular list of function pointers.
- */
- if ((s->rep->analysers & ~last_ana) & AN_RES_WAIT_HTTP)
- continue;
+ UPDATE_ANALYSERS(s->rep->analysers, ana_list, ana_back, AN_RES_HTTP_PROCESS_BE);
}
- if (s->rep->analysers & AN_RES_HTTP_XFER_BODY) {
- last_ana |= AN_RES_HTTP_XFER_BODY;
+ if (ana_list & AN_RES_HTTP_XFER_BODY) {
if (!http_response_forward_body(s, s->rep, AN_RES_HTTP_XFER_BODY))
break;
+ UPDATE_ANALYSERS(s->rep->analysers, ana_list, ana_back, AN_RES_HTTP_XFER_BODY);
}
- }
+ } while (0);
}
if ((s->rep->flags ^ flags) & BF_MASK_STATIC) {