Le 16/09/2023 à 16:00, Valters Jansons a écrit :
On Thu, Sep 14, 2023 at 12:35 PM Christopher Faulet <cfau...@haproxy.com> wrote:
After a discussion with Willy, we've hopefully found a way to fix the issue by
delaying detection of the server abort on the request processing side when there
is a response to forward to the client. It should do the trick in your case and
it should be safe.
However, the fix remains sensitive. It is really hard to be sure it will not
introduce a regression. The worst could be to block a session or to get a loop
because of an unhandled event.
Thus it could be go to test it on your side if it is possible. The patch is in
attachment. It can be applied on top of the 2.9 or 2.8. Is this possible for
you ?
I applied the patch on top of v2.9-dev5. For what it's worth, I build
on Ubuntu 22.04 with defaults from INSTALL: TARGET=linux-glibc
USE_OPENSSL=1 USE_LUA=1 USE_PCRE=1 USE_SYSTEMD=1.
The delayed processing patch does not appear to change anything for
me. I am still seeing HTTP 502 (Bad Gateway) and HAProxy logs report
session state as SH--. According to documentation, that is "the TCP
session was unexpectedly aborted by the server, or the server
explicitly refused it" together with "the proxy was waiting for
complete, valid response HEADERS from the server (HTTP only)".
Somehow, if the H2 connection to backend is half-open(local), HAProxy
is expecting more HEADERS from the backend server.
I am providing a short diff below, which force-closes the H2
connection to backend upon remote ES (which would break long-running
upload from frontend to client). The change is interesting, because it
highlights something weird with processing. Without this change, even
with your provided patch, HAProxy never finishes processing HEADERS.
However, with this change, response is delivered to the frontend
client when the stream closes.
The session state is probably not relevant, but for what it's worth,
it becomes CL--. C for "the TCP session was unexpectedly aborted by
the client" -- not sure why unexpectedly, because from the client side
everything received successfully -- and L for "the proxy was still
transmitting LAST data to the client while the server had already
finished. This one is very rare as it can only happen when the client
dies while receiving the last packets". Maybe this gives some other
ideas.
diff --git a/src/mux_h2.c b/src/mux_h2.c
index cc698b66b..c1ff014dd 100644
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -2934,7 +2934,7 @@ static struct h2s *h2c_bck_handle_headers(struct
h2c *h2c, struct h2s *h2s)
else if (h2s->flags & H2_SF_ES_RCVD) {
if (h2s->st == H2_SS_OPEN)
h2s->st = H2_SS_HREM;
- else if (h2s->st == H2_SS_HLOC)
+ if (h2s->st == H2_SS_HLOC || h2s->st == H2_SS_HREM)
h2s_close(h2s);
}
The clients should send proper and timely END_STREAMs, so this should
not be a major issue, but something still feels strange here with
connection state processing.
Let me know if I can help brainstorm this further.
I'm really sorry. Of course it does not solve your issue. I failed the fix by
inverting the condition on the response buffer ! Thus it does the opposite of
what we want to do...
Please find a new patch. It should be good ( or better at least :)
--
Christopher Faulet
From 911b5d36c035972e9c2c82cac69afe343bc5c9fa Mon Sep 17 00:00:00 2001
From: Christopher Faulet <cfau...@haproxy.com>
Date: Thu, 14 Sep 2023 11:12:32 +0200
Subject: [PATCH] BUG/MEDIUM: http-ana: Try to handle response before handling
server abort
In the request analyser responsible to forward the request, we try to detect
the server abort to stop the request forwarding. However, we must be careful
to not block the response processing, if any. Indeed, it is possible to get
the response and the server abort in same time. In this case, we must try to
forward the response to the client first.
So to fix the issue, in the request analyser we no longer handle the server
abort if the response channel is not empty. In the end, the response
analyser is able to detect the server abort if it is relevant. Otherwise,
the stream will be woken up after the response forwarding and the server
abort should be handled at this stage.
This patch should be backported as far as 2.7 only because the risk of
breakage is high. And it is probably a good idea to wait a bit before
backporting it.
---
src/http_ana.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/src/http_ana.c b/src/http_ana.c
index 819472c17..be75eac44 100644
--- a/src/http_ana.c
+++ b/src/http_ana.c
@@ -985,8 +985,12 @@ int http_request_forward_body(struct stream *s, struct channel *req, int an_bit)
if ((s->scb->flags & SC_FL_SHUT_DONE) && co_data(req)) {
/* request errors are most likely due to the server aborting the
- * transfer. */
- goto return_srv_abort;
+ * transfer.Bit handle server aborts only if there is no
+ * response. Otherwise, let a change to foward the response
+ * first.
+ */
+ if (htx_is_empty(htxbuf(&s->res.buf)))
+ goto return_srv_abort;
}
http_end_request(s);
@@ -1023,8 +1027,13 @@ int http_request_forward_body(struct stream *s, struct channel *req, int an_bit)
waiting:
/* waiting for the last bits to leave the buffer */
- if (s->scb->flags & SC_FL_SHUT_DONE)
- goto return_srv_abort;
+ if (s->scb->flags & SC_FL_SHUT_DONE) {
+ /* Handle server aborts only if there is no response. Otherwise,
+ * let a change to foward the response first.
+ */
+ if (htx_is_empty(htxbuf(&s->res.buf)))
+ goto return_srv_abort;
+ }
/* 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.
--
2.41.0
