From: Christopher Faulet <cfaulet@haproxy.com>
Date: Thu, 6 Jul 2017 13:53:02 +0000 (+0200)
Subject: BUG/MEDIUM: filters: Be sure to call flt_end_analyze for both channels
X-Git-Tag: v1.8-dev3~249
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=570f79987756b1534d5601b66ba68602a847ec09;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: filters: Be sure to call flt_end_analyze for both channels

In the commit 2b553de5 ("BUG/MINOR: filters: Don't force the stream's wakeup
when we wait in flt_end_analyze"), we removed a task_wakeup in flt_end_analyze
to no consume too much CPU by looping in certain circumstances.

But this fix was too drastic. For Keep-Alive transactions, flt_end_analyze is
often called only for the response. Then the stream is paused until a timeout is
hitted or the next request is received. We need first let a chance to both
channels to call flt_end_analyze function. Then if a filter need to wait here,
it is its responsibility to wake up the stream when needed. To fix the bug, and
thanks to previous commits, we set the flag CF_WAKE_ONCE on channels to pretend
there is an activity. On the current channel, the flag will be removed without
any effect, but for the other side the analyzer will be called immediatly.

Thanks for Lukas Tribus for his detailed analysis of the bug.

This patch must be backported in 1.7 with the 2 previous ones:

  * a94fda3 ("BUG/MINOR: http: Don't reset the transaction if there are still data to send")
  * cdaea89 ("BUG/MINOR: stream: Don't forget to remove CF_WAKE_ONCE flag on response channel")
---

diff --git a/src/filters.c b/src/filters.c
index 68bf7ee527..0d0e5e8ced 100644
--- a/src/filters.c
+++ b/src/filters.c
@@ -843,9 +843,21 @@ flt_end_analyze(struct stream *s, struct channel *chn, unsigned int an_bit)
 	/* We don't remove yet this analyzer because we need to synchronize the
 	 * both channels. So here, we just remove the flag CF_FLT_ANALYZE. */
 	ret = handle_analyzer_result(s, chn, 0, ret);
-	if (ret)
+	if (ret) {
 		chn->flags &= ~CF_FLT_ANALYZE;
 
+		/* Pretend there is an activity on both channels. Flag on the
+		 * current one will be automatically removed, so only the other
+		 * one will remain. This is a way to be sure that
+		 * 'channel_end_analyze' callback will have a chance to be
+		 * called at least once for the other side to finish the current
+		 * processing. Of course, this is the filter responsiblity to
+		 * wakeup the stream if it choose to loop on this callback. */
+		s->req.flags |= CF_WAKE_ONCE;
+		s->res.flags |= CF_WAKE_ONCE;
+	}
+
+
  sync:
 	/* Now we can check if filters have finished their work on the both
 	 * channels */