BUG/MEDIUM: http: don't automatically forward request close
Maximilian Böhm, and Lucas Rolff reported some frequent HTTP/2 POST
failures affecting version 1.8.2 that were not affecting 1.8.1. Lukas
Tribus determined that these ones appeared consecutive to commit a48c141
("BUG/MAJOR: connection: refine the situations where we don't send shutw()").
It turns out that the HTTP request forwarding engine lets a shutr from
the client be automatically forwarded to the server unless chunked
encoding is in use. It's a bit tricky to meet this condition as it only
happens if the shutr is not reported in the initial request. So if a
request is large enough or the body is delayed after the headers (eg:
Expect: 100-continue), the the function quits with channel_auto_close()
left enabled. The patch above was not really related in fact. It's just
that a previous bug was causing this shutw to be skipped at the lower
layers, and the two bugs used to cancel themselves.
In the HTTP request we should only pass the close in tunnel mode, as
other cases either need to keep the connection alive (eg: for reuse)
or will force-close it. Also the forced close will properly take care
of avoiding the painful time-wait, which is not possible with the early
close.
This patch must be backported to 1.8 as it directly impacts HTTP/2, and
may be backported to older version to save them from being abused by
clients causing TIME_WAITs between haproxy and the server.
Thanks to Lukas and Lucas for running many tests with captures allowing
the bug to be narrowed down.
diff --git a/src/proto_http.c b/src/proto_http.c
index f585dee..64bd410 100644
--- a/src/proto_http.c
+++ b/src/proto_http.c
@@ -4963,8 +4963,13 @@
/* When TE: chunked is used, we need to get there again to parse remaining
* chunks even if the client has closed, so we don't want to set CF_DONTCLOSE.
+ * And when content-length is used, we never want to let the possible
+ * shutdown be forwarded to the other side, as the state machine will
+ * take care of it once the client responds. It's also important to
+ * prevent TIME_WAITs from accumulating on the backend side, and for
+ * HTTP/2 where the last frame comes with a shutdown.
*/
- if (msg->flags & HTTP_MSGF_TE_CHNK)
+ if (msg->flags & (HTTP_MSGF_TE_CHNK|HTTP_MSGF_CNT_LEN))
channel_dont_close(req);
/* We know that more data are expected, but we couldn't send more that