Re: [PATCH] MINOR: proxy: add option idle-close-on-response

2022-01-06 Thread Willy Tarreau
On Wed, Jan 05, 2022 at 10:53:24PM +0100, William Dauchy wrote:
> Avoid closing idle connections if a soft stop is in progress.
> 
> By default, idle connections will be closed during a soft stop. In some
> environments, a client talking to the proxy may have prepared some idle
> connections in order to send requests later. If there is no proper retry
> on write errors, this can result in errors while haproxy is reloading.
> Even though a proper implementation should retry on connection/write
> errors, this option was introduced to support back compat with haproxy <
> v2.4. Indeed before v2.4, we were waiting for a last request to be able
> to add a "connection: close" header and advice the client to close the
> connection.
(...)

Perfect, thank you William! I've just added a small paragraph to the
doc mentioning the impact of using this for those with long idle
connections, and pointers to the relevant options timeout client,
timeout http-request and hard-stop-after. I could also have added
timeout http-keep-alive by the way. Regardless all of them are
cross-references, that should be enough.

Now merged, thank you!

Willy



[PATCH] MINOR: proxy: add option idle-close-on-response

2022-01-05 Thread William Dauchy
Avoid closing idle connections if a soft stop is in progress.

By default, idle connections will be closed during a soft stop. In some
environments, a client talking to the proxy may have prepared some idle
connections in order to send requests later. If there is no proper retry
on write errors, this can result in errors while haproxy is reloading.
Even though a proper implementation should retry on connection/write
errors, this option was introduced to support back compat with haproxy <
v2.4. Indeed before v2.4, we were waiting for a last request to be able
to add a "connection: close" header and advice the client to close the
connection.

In a real life example, this behavior was seen in AWS using the ALB in
front of a haproxy. The end result was ALB sending 502 during haproxy
reloads.
This patch was tested on haproxy v2.4, with a regular reload on the
process, and a constant trend of requests coming in. Before the patch,
we see regular 502 returned to the client; when activating the option,
the 502 disappear.

This patch should help fixing github issue #1506.
In order to unblock some v2.3 to v2.4 migraton, this patch should be
backported up to v2.4 branch.

Signed-off-by: William Dauchy 
---
 doc/configuration.txt | 21 +
 include/haproxy/proxy-t.h |  2 +-
 src/mux_h1.c  |  3 ++-
 src/proxy.c   |  1 +
 4 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/doc/configuration.txt b/doc/configuration.txt
index cd8ab4b72..66571a566 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -3756,6 +3756,7 @@ option tcp-smart-connect (*)  X  -
 X X
 option tcpka  X  X X X
 option tcplog X  X X X
 option transparent   (*)  X  - X X
+option idle-close-on-response(*)  X  X X -
 external-check commandX  - X X
 external-check path   X  - X X
 persist rdp-cookieX  - X X
@@ -9836,6 +9837,26 @@ no option transparent
 "transparent" option of the "bind" keyword.
 
 
+option idle-close-on-response
+no option idle-close-on-response
+  Avoid closing idle connections if a stop stop is in progress
+  May be used in sections :   defaults | frontend | listen | backend
+ yes   |yes   |   yes  |   no
+  Arguments : none
+
+  By default, idle connections will be closed during a soft stop. In some
+  environments, a client talking to the proxy may have prepared some idle
+  connections in order to send requests later. If there is no proper retry on
+  write errors, this can result in errors while haproxy is reloading. Even
+  though a proper implementation should retry on connection/write errors, this
+  option was introduced to support back compat with haproxy < v2.4. Indeed
+  before v2.4, we were waiting for a last request to be able to add a
+  "connection: close" header and advice the client to close the connection.
+
+  In a real life example, this behavior was seen in AWS using the ALB in front
+  of a haproxy. The end result was ALB sending 502 during haproxy reloads.
+
+
 external-check command 
   Executable to run when performing an external-check
   May be used in sections :   defaults | frontend | listen | backend
diff --git a/include/haproxy/proxy-t.h b/include/haproxy/proxy-t.h
index 8ca4e818d..421f900e2 100644
--- a/include/haproxy/proxy-t.h
+++ b/include/haproxy/proxy-t.h
@@ -82,7 +82,7 @@ enum PR_SRV_STATE_FILE {
 #define PR_O_REUSE_ALWS 0x000C  /* always reuse a shared connection */
 #define PR_O_REUSE_MASK 0x000C  /* mask to retrieve shared connection 
preferences */
 
-/* unused: 0x10 */
+#define PR_O_IDLE_CLOSE_RESP 0x0010 /* avoid closing idle connections 
during a soft stop */
 #define PR_O_PREF_LAST  0x0020  /* prefer last server */
 #define PR_O_DISPATCH   0x0040  /* use dispatch mode */
 #define PR_O_FORCED_ID  0x0080  /* proxy's ID was forced in the 
configuration */
diff --git a/src/mux_h1.c b/src/mux_h1.c
index 7b6ab946d..1ec6cb77c 100644
--- a/src/mux_h1.c
+++ b/src/mux_h1.c
@@ -2999,7 +2999,8 @@ static int h1_process(struct h1c * h1c)
 */
if (!(h1c->flags & H1C_F_IS_BACK)) {
if (unlikely(h1c->px->flags & (PR_FL_DISABLED|PR_FL_STOPPED))) {
-   if (h1c->flags & H1C_F_WAIT_NEXT_REQ)
+   if (!(h1c->px->options & PR_O_IDLE_CLOSE_RESP) &&
+   h1c->flags & H1C_F_WAIT_NEXT_REQ)
goto release;
}
}
diff --git a/src/proxy.c b/src/proxy.c
index ff5e35e2c..245b6f8a5 100644
--- a/src/proxy.c
+++ b/src/proxy.c
@@ -80,6 +80,7 @@ const struct cfg_opt cfg_opts[] =