BUG/MINOR: server: fix logic flaw in idle connection list management

With variable connection limits, it's not possible to accurately determine
whether the mux is still in use by comparing usage and max to be equal due
to the fact that one determines the capacity and the other one takes care
of the context. This can cause some connections to be dropped before they
reach their stream ID limit.

It seems it could also cause some connections to be terminated with
streams still alive if the limit was reduced to match the newly computed
avail_streams() value, though this cannot yet happen with existing muxes.

Instead let's switch to usage reports and simply check whether connections
are both unused and available before adding them to the idle list.

This should be backported to 1.9.

diff --git a/include/proto/server.h b/include/proto/server.h
index 436ffb5d5ef78b6bd328a26b389232fc0120de19..7aa09e4d9b793db83e6416eb6a06b7f304ef172e 100644 (file)
--- a/include/proto/server.h
+++ b/include/proto/server.h
@@ -233,13 +233,16 @@ static inline enum srv_initaddr srv_get_next_initaddr(unsigned int *list)
        return ret;
 }
 
+/* This adds an idle connection to the server's list if the connection is
+ * reusable, not held by any owner anymore, but still has available streams.
+ */
 static inline int srv_add_to_idle_list(struct server *srv, struct connection *conn)
 {
-       if (srv && srv->pool_purge_delay > 0 && (srv->max_idle_conns == -1 ||
-           srv->max_idle_conns > srv->curr_idle_conns) &&
+       if (srv && srv->pool_purge_delay > 0 &&
+           (srv->max_idle_conns == -1 || srv->max_idle_conns > srv->curr_idle_conns) &&
            !(conn->flags & CO_FL_PRIVATE) &&
            ((srv->proxy->options & PR_O_REUSE_MASK) != PR_O_REUSE_NEVR) &&
-           conn->mux->avail_streams(conn) == conn->mux->max_streams(conn)) {
+           !conn->mux->used_streams(conn) && conn->mux->avail_streams(conn)) {
                LIST_DEL(&conn->list);
                LIST_ADDQ(&srv->idle_orphan_conns[tid], &conn->list);
                srv->curr_idle_conns++;

diff --git a/include/types/connection.h b/include/types/connection.h
index f11cfb531d3007a410a95b70269e512279acfaa1..96728b35f5ecd0f45c7554a526f42756568987cc 100644 (file)
--- a/include/types/connection.h
+++ b/include/types/connection.h
@@ -342,7 +342,7 @@ struct mux_ops {
        int (*subscribe)(struct conn_stream *cs, int event_type, void *param); /* Subscribe to events, such as "being able to send" */
        int (*unsubscribe)(struct conn_stream *cs, int event_type, void *param); /* Unsubscribe to events */
        int (*avail_streams)(struct connection *conn); /* Returns the number of streams still available for a connection */
-       int (*max_streams)(struct connection *conn);   /* Returns the max number of streams available for that connection. */
+       int (*used_streams)(struct connection *conn);  /* Returns the number of streams in use on a connection. */
        void (*destroy)(struct connection *conn); /* Let the mux know one of its users left, so it may have to disappear */
        void (*reset)(struct connection *conn); /* Reset the mux, because we're re-trying to connect */
        const struct cs_info *(*get_cs_info)(struct conn_stream *cs); /* Return info on the specified conn_stream or NULL if not defined */

diff --git a/src/mux_h1.c b/src/mux_h1.c
index cce0c44a69ffdc11fada57f2ec0ed914e4d44d10..ead67e06e2a316e1bee8aaf8442997b2f043eae9 100644 (file)
--- a/src/mux_h1.c
+++ b/src/mux_h1.c
@@ -208,18 +208,24 @@ static inline void h1_release_buf(struct h1c *h1c, struct buffer *bptr)
        }
 }
 
-static int h1_avail_streams(struct connection *conn)
+/* returns the number of streams in use on a connection to figure if it's
+ * idle or not. We can't have an h1s without a CS so checking h1s is fine,
+ * as the caller will want to know if it was the last one after a detach().
+ */
+static int h1_used_streams(struct connection *conn)
 {
        struct h1c *h1c = conn->ctx;
 
-       return h1c->h1s ? 0 : 1;
+       return h1c->h1s ? 1 : 0;
 }
 
-static int h1_max_streams(struct connection *conn)
+/* returns the number of streams still available on a connection */
+static int h1_avail_streams(struct connection *conn)
 {
-       return 1;
+       return 1 - h1_used_streams(conn);
 }
 
+
 /*****************************************************************/
 /* functions below are dedicated to the mux setup and management */
 /*****************************************************************/
@@ -2349,7 +2355,7 @@ static const struct mux_ops mux_h1_ops = {
        .detach      = h1_detach,
        .destroy     = h1_destroy,
        .avail_streams = h1_avail_streams,
-       .max_streams = h1_max_streams,
+       .used_streams = h1_used_streams,
        .rcv_buf     = h1_rcv_buf,
        .snd_buf     = h1_snd_buf,
 #if defined(CONFIG_HAP_LINUX_SPLICE)

diff --git a/src/mux_h2.c b/src/mux_h2.c
index f20d0b094097e81706d51eea42fa2a4f8c7a4a70..6b4ba611143d6f38d3a004df0fc9b76e8d269d60 100644 (file)
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -408,6 +408,17 @@ static inline int h2_streams_left(const struct h2c *h2c)
        return ret;
 }
 
+/* returns the number of streams in use on a connection to figure if it's
+ * idle or not. We check nb_cs and not nb_streams as the caller will want
+ * to know if it was the last one after a detach().
+ */
+static int h2_used_streams(struct connection *conn)
+{
+       struct h2c *h2c = conn->ctx;
+
+       return h2c->nb_cs;
+}
+
 /* returns the number of concurrent streams available on the connection */
 static int h2_avail_streams(struct connection *conn)
 {
@@ -434,12 +445,6 @@ static int h2_avail_streams(struct connection *conn)
        return ret1;
 }
 
-static int h2_max_streams(struct connection *conn)
-{
-       /* XXX Should use the negociated max concurrent stream nb instead of the conf value */
-       return h2_settings_max_concurrent_streams;
-}
-
 
 /*****************************************************************/
 /* functions below are dedicated to the mux setup and management */
@@ -5444,7 +5449,7 @@ static const struct mux_ops h2_ops = {
        .detach = h2_detach,
        .destroy = h2_destroy,
        .avail_streams = h2_avail_streams,
-       .max_streams = h2_max_streams,
+       .used_streams = h2_used_streams,
        .shutr = h2_shutr,
        .shutw = h2_shutw,
        .show_fd = h2_show_fd,

diff --git a/src/mux_pt.c b/src/mux_pt.c
index da1c343a7ee491ded7bc08406d3a98d4589fdc16..462a7593aaaabe4bdae0ee3cbf9f39b5daf997bf 100644 (file)
--- a/src/mux_pt.c
+++ b/src/mux_pt.c
@@ -193,16 +193,18 @@ static void mux_pt_detach(struct conn_stream *cs)
                mux_pt_destroy(ctx);
 }
 
-static int mux_pt_avail_streams(struct connection *conn)
+/* returns the number of streams in use on a connection */
+static int mux_pt_used_streams(struct connection *conn)
 {
        struct mux_pt_ctx *ctx = conn->ctx;
 
-       return (ctx->cs == NULL ? 1 : 0);
+       return ctx->cs ? 1 : 0;
 }
 
-static int mux_pt_max_streams(struct connection *conn)
+/* returns the number of streams still available on a connection */
+static int mux_pt_avail_streams(struct connection *conn)
 {
-       return 1;
+       return 1 - mux_pt_used_streams(conn);
 }
 
 static void mux_pt_shutr(struct conn_stream *cs, enum cs_shr_mode mode)
@@ -325,7 +327,7 @@ const struct mux_ops mux_pt_ops = {
        .get_first_cs = mux_pt_get_first_cs,
        .detach = mux_pt_detach,
        .avail_streams = mux_pt_avail_streams,
-       .max_streams = mux_pt_max_streams,
+       .used_streams = mux_pt_used_streams,
        .destroy = mux_pt_destroy_meth,
        .shutr = mux_pt_shutr,
        .shutw = mux_pt_shutw,