Le 14/09/2023 à 01:36, Valters Jansons a écrit :
I set up a small PoC repository at https://github.com/sigv/grpcopen
with a server and a client. There is a Ping endpoint, which works fine
(the frontend client is first to close). There is also a Foobar
endpoint, which is intentionally mangled, to ensure the backend server
returns a gRPC error before the frontend client closes its side.
Currently `client/main.go` sends both requests; to only observe the
failing request, `ping(stream, ctx)` invocation can be removed from
the end of the file.
The only requirement to run it is to have Go available locally. The
README file in the repository covers how to download and unpack Go, if
you want to run this on a fresh VM. It also includes a minimal HAProxy
configuration that I can reproduce the issue with.
Hopefully having a sample application locally makes it easier for you
to look at the raw traffic, and trace HAProxy itself.
Please let me know if I can help out in any other way!
Hi,
First of all, thanks for your reproducer. It is really helpful.
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 ?
--
Christopher Faulet
From 04892caae72eb13605e4a32b4a182ec22fcc30bf Mon Sep 17 00:00:00 2001
From: Christopher Faulet
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..2d2ce90d1 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(>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(>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
