From 4ad1c9635a9cbffbd4fd89bd7b3f013d01cadbeb Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Fri, 2 Jun 2023 16:19:51 +0200 Subject: [PATCH] BUG/MINOR: stream: do not use client-fin/server-fin with HTX Historically the client-fin and server-fin timeouts were made to allow a connection closure to be effective quickly if the last data were sent down a socket and the client didn't close, something that can happen when the peer's FIN is lost and retransmits are blocked by a firewall for example. This made complete sense in 1.5 for TCP and HTTP in close mode. But nowadays with muxes, it's not done at the right layer anymore and even the description doesn't match what is being done, because what happens is that the stream will abort the whole transfer after it's done sending to the mux and this timeout expires. We've seen in GH issue 2095 that this can happen with very short timeout values, and while this didn't trigger often before, now that the muxes (h2 & quic) properly report an end of stream before even the first sc_conn_sync_recv(), it seems that it can happen more often, and have two undesirable effects: - logging a timeout when that's not the case - aborting the request channel, hence the server-side conn, possibly before it had a chance to be put back to the idle list, causing this connection to be closed and not reusable. Unfortunately for TCP (mux_pt) this remains necessary because the mux doesn't have a timeout task. So here we're adding tests to only do this through an HTX mux. But to be really clean we should in fact completely drop all of this and implement these timeouts in the mux itself. This needs to be backported to 2.8 where the issue was discovered, and maybe carefully to older versions, though that is not sure at all. In any case, using a higher timeout or removing client-fin in HTTP proxies is sufficient to make the issue disappear. --- include/haproxy/sc_strm.h | 3 +++ src/stream.c | 10 ++++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/include/haproxy/sc_strm.h b/include/haproxy/sc_strm.h index 8dc0d84b39..7555fbf8a3 100644 --- a/include/haproxy/sc_strm.h +++ b/include/haproxy/sc_strm.h @@ -389,6 +389,9 @@ static inline void sc_set_hcto(struct stconn *sc) { struct stream *strm = __sc_strm(sc); + if (IS_HTX_STRM(strm)) + return; + if (sc->flags & SC_FL_ISBACK) { if ((strm->flags & SF_BE_ASSIGNED) && tick_isset(strm->be->timeout.serverfin)) sc->ioto = strm->be->timeout.serverfin; diff --git a/src/stream.c b/src/stream.c index 4bcc545a15..ef35b6e3c7 100644 --- a/src/stream.c +++ b/src/stream.c @@ -2453,10 +2453,12 @@ struct task *process_stream(struct task *t, void *context, unsigned int state) if (!req->analysers && s->tunnel_timeout) { scf->ioto = scb->ioto = s->tunnel_timeout; - if ((scf->flags & (SC_FL_EOS|SC_FL_ABRT_DONE|SC_FL_SHUT_DONE)) && tick_isset(sess->fe->timeout.clientfin)) - scf->ioto = sess->fe->timeout.clientfin; - if ((scb->flags & (SC_FL_EOS|SC_FL_ABRT_DONE|SC_FL_SHUT_DONE)) && tick_isset(s->be->timeout.serverfin)) - scb->ioto = s->be->timeout.serverfin; + if (!IS_HTX_STRM(s)) { + if ((scf->flags & (SC_FL_EOS|SC_FL_ABRT_DONE|SC_FL_SHUT_DONE)) && tick_isset(sess->fe->timeout.clientfin)) + scf->ioto = sess->fe->timeout.clientfin; + if ((scb->flags & (SC_FL_EOS|SC_FL_ABRT_DONE|SC_FL_SHUT_DONE)) && tick_isset(s->be->timeout.serverfin)) + scb->ioto = s->be->timeout.serverfin; + } } }