BUG/MEDIUM: mux-h2: make use of http-request and keep-alive timeouts
Christian Ruppert reported an issue explaining that it's not possible to
forcefully close H2 connections which do not receive requests anymore if
they continue to send control traffic (window updates, ping etc). This
will indeed refresh the timeout. In H1 we don't have this problem because
any single byte is part of the stream, so the control frames in H2 would
be equivalent to TCP acks in H1, that would not contribute to the timeout
being refreshed.
What misses from H2 is the use of http-request and keep-alive timeouts.
These were not implemented because initially it was hard to see how they
could map to H2. But if we consider the real use of the keep-alive timeout,
that is, how long do we keep a connection alive with no request, then it's
pretty obvious that it does apply to H2 as well. Similarly, http-request
may definitely be honored as soon as a HEADERS frame starts to appear
while there is no stream. This will also allow to deal with too long
CONTINUATION frames.
This patch moves the timeout update to a new function, h2c_update_timeout(),
which is in charge of this. It also adds an "idle_start" timestamp in the
connection, which is set when nb_cs reaches zero or when a headers frame
start to arrive, so that it cannot be delayed too long.
This patch should be backported to recent stable releases after some
observation time. It depends on previous patch "MEDIUM: mux-h2: slightly
relax timeout management rules".
diff --git a/src/mux_h2.c b/src/mux_h2.c
index e5ab74c..dd1137f 100644
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -133,6 +133,8 @@
int timeout; /* idle timeout duration in ticks */
int shut_timeout; /* idle timeout duration in ticks after GOAWAY was sent */
+ int idle_start; /* date of the last time the connection went idle */
+ /* 32-bit hole here */
unsigned int nb_streams; /* number of streams in the tree */
unsigned int nb_cs; /* number of attached conn_streams */
unsigned int nb_reserved; /* number of reserved streams */
@@ -701,6 +703,43 @@
return !h2c->nb_cs;
}
+/* update h2c timeout if needed */
+static void h2c_update_timeout(struct h2c *h2c)
+{
+ TRACE_ENTER(H2_EV_H2C_WAKE, h2c->conn);
+
+ if (!h2c->task)
+ goto leave;
+
+ if (h2c_may_expire(h2c)) {
+ /* no more streams attached */
+ if (h2c->last_sid >= 0) {
+ /* GOAWAY sent, closing in progress */
+ h2c->task->expire = tick_add_ifset(now_ms, h2c->shut_timeout);
+ } else if (br_data(h2c->mbuf)) {
+ /* pending output data: always the regular data timeout */
+ h2c->task->expire = tick_add_ifset(now_ms, h2c->timeout);
+ } else if (h2c->max_id > 0 && !b_data(&h2c->dbuf)) {
+ /* idle after having seen one stream => keep-alive */
+ h2c->task->expire = tick_add_ifset(h2c->idle_start, h2c->proxy->timeout.httpka);
+ } else {
+ /* before first request, or started to deserialize a
+ * new req => http-request, but only set, not refresh.
+ */
+ int exp = (h2c->flags & H2_CF_IS_BACK) ? TICK_ETERNITY : h2c->proxy->timeout.httpreq;
+ h2c->task->expire = tick_add_ifset(h2c->idle_start, exp);
+ }
+ /* if a timeout above was not set, fall back to the default one */
+ if (!tick_isset(h2c->task->expire))
+ h2c->task->expire = tick_add_ifset(now_ms, h2c->timeout);
+ } else {
+ h2c->task->expire = TICK_ETERNITY;
+ }
+ task_queue(h2c->task);
+ leave:
+ TRACE_LEAVE(H2_EV_H2C_WAKE);
+}
+
static __inline int
h2c_is_dead(const struct h2c *h2c)
{
@@ -942,6 +981,7 @@
h2c->proxy = prx;
h2c->task = NULL;
+ h2c->idle_start = now_ms;
if (tick_isset(h2c->timeout)) {
t = task_new_here();
if (!t)
@@ -1562,6 +1602,8 @@
out_free_cs:
h2c->nb_cs--;
+ if (!h2c->nb_cs)
+ h2c->idle_start = now_ms;
cs_free(cs);
h2s->cs = NULL;
out_close:
@@ -3356,6 +3398,12 @@
HA_ATOMIC_INC(&h2c->px_counters->conn_proto_err);
goto done;
}
+
+ /* transition to HEADERS frame ends the keep-alive idle
+ * timer and starts the http-request idle delay.
+ */
+ if (hdr.ft == H2_FT_HEADERS)
+ h2c->idle_start = now_ms;
}
/* Only H2_CS_FRAME_P, H2_CS_FRAME_A and H2_CS_FRAME_E here.
@@ -4024,14 +4072,7 @@
((h2c->flags & H2_CF_MUX_BLOCK_ANY) || LIST_ISEMPTY(&h2c->send_list))))
h2_release_mbuf(h2c);
- if (h2c->task) {
- if (h2c_may_expire(h2c))
- h2c->task->expire = tick_add(now_ms, h2c->last_sid < 0 ? h2c->timeout : h2c->shut_timeout);
- else
- h2c->task->expire = TICK_ETERNITY;
- task_queue(h2c->task);
- }
-
+ h2c_update_timeout(h2c);
h2_send(h2c);
TRACE_LEAVE(H2_EV_H2C_WAKE, conn);
return 0;
@@ -4270,6 +4311,9 @@
h2c = h2s->h2c;
h2s->cs = NULL;
h2c->nb_cs--;
+ if (!h2c->nb_cs)
+ h2c->idle_start = now_ms;
+
if ((h2c->flags & (H2_CF_IS_BACK|H2_CF_DEM_TOOMANY)) == H2_CF_DEM_TOOMANY &&
!h2_frt_has_too_many_cs(h2c)) {
/* frontend connection was blocking new streams creation */
@@ -4285,6 +4329,11 @@
(h2s->flags & (H2_SF_BLK_MBUSY | H2_SF_BLK_MROOM | H2_SF_BLK_MFCTL)) &&
((h2s->flags & (H2_SF_WANT_SHUTR | H2_SF_WANT_SHUTW)) || h2s->subs)) {
TRACE_DEVEL("leaving on stream blocked", H2_EV_STRM_END|H2_EV_H2S_BLK, h2c->conn, h2s);
+ /* refresh the timeout if none was active, so that the last
+ * leaving stream may arm it.
+ */
+ if (!tick_isset(h2c->task->expire))
+ h2c_update_timeout(h2c);
return;
}
@@ -4373,11 +4422,7 @@
h2_release(h2c);
}
else if (h2c->task) {
- if (h2c_may_expire(h2c))
- h2c->task->expire = tick_add(now_ms, h2c->last_sid < 0 ? h2c->timeout : h2c->shut_timeout);
- else
- h2c->task->expire = TICK_ETERNITY;
- task_queue(h2c->task);
+ h2c_update_timeout(h2c);
TRACE_DEVEL("leaving, refreshing connection's timeout", H2_EV_STRM_END, h2c->conn);
}
else