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.

diff --git a/include/haproxy/sc_strm.h b/include/haproxy/sc_strm.h
index 8dc0d84b39968c05a3faf7679a2774364d96d3a8..7555fbf8a3d3215b079e0f65c8e258c9349be29e 100644 (file)
--- 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 4bcc545a15f2346a0b29aacd9d9c25bf8d349082..ef35b6e3c7b0b43830d858bde41828ee275533d9 100644 (file)
--- 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;
+                       }
                }
        }