BUG/MEDIUM: mux-h2: do not enforce timeout on long connections
Alexandre Derumier reported issue #308 in which the client timeout will
strike on an H2 mux when it's shorter than the server's response time.
What happens in practice is that there is no activity on the connection
and there's no data pending on output so we can expire it. But this does
not take into account the possibility that some streams are in fact
waiting for the data layer above. So what we do now is that we enforce
the timeout when:
- there are no more streams
- some data are pending in the output buffer
- some streams are blocked on the connection's flow control
- some streams are blocked on their own flow control
- some streams are in the send/sending list
In all other cases the connection will not timeout as it means that some
streams are actively used by the data layer.
This fix must be backported to 2.0, 1.9 and probably 1.8 as well. It
depends on the new "blocked_list" field introduced by "MINOR: mux-h2:
add a per-connection list of blocked streams". It would be nice to
also backport "ebtree: make eb_is_empty() and eb_is_dup() take a const"
to avoid a build warning.
diff --git a/src/mux_h2.c b/src/mux_h2.c
index 94d387a..2c295fc 100644
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -548,8 +548,28 @@
}
}
+/* returns true if the connection is allowed to expire, false otherwise. A
+ * connection may expire when:
+ * - it has no stream
+ * - it has data in the mux buffer
+ * - it has streams in the blocked list
+ * - it has streams in the fctl list
+ * - it has streams in the send list
+ * Otherwise it means some streams are waiting in the data layer and it should
+ * not expire.
+ */
+static inline int h2c_may_expire(const struct h2c *h2c)
+{
+ return eb_is_empty(&h2c->streams_by_id) ||
+ br_data(h2c->mbuf) ||
+ !LIST_ISEMPTY(&h2c->blocked_list) ||
+ !LIST_ISEMPTY(&h2c->fctl_list) ||
+ !LIST_ISEMPTY(&h2c->send_list) ||
+ !LIST_ISEMPTY(&h2c->sending_list);
+}
+
static __inline int
-h2c_is_dead(struct h2c *h2c)
+h2c_is_dead(const struct h2c *h2c)
{
if (eb_is_empty(&h2c->streams_by_id) && /* don't close if streams exist */
((h2c->conn->flags & CO_FL_ERROR) || /* errors close immediately */
@@ -3551,7 +3571,10 @@
h2_release_mbuf(h2c);
if (h2c->task) {
- h2c->task->expire = tick_add(now_ms, h2c->last_sid < 0 ? h2c->timeout : h2c->shut_timeout);
+ 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);
}
@@ -3590,6 +3613,14 @@
return t;
}
+ if (h2c && !h2c_may_expire(h2c)) {
+ /* we do still have streams but all of them are idle, waiting
+ * for the data layer, so we must not enforce the timeout here.
+ */
+ t->expire = TICK_ETERNITY;
+ return t;
+ }
+
task_destroy(t);
if (!h2c) {
@@ -3829,7 +3860,10 @@
h2_release(h2c);
}
else if (h2c->task) {
- h2c->task->expire = tick_add(now_ms, h2c->last_sid < 0 ? h2c->timeout : h2c->shut_timeout);
+ 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);
TRACE_DEVEL("leaving, refreshing connection's timeout", H2_EV_STRM_END, h2c->conn);
}