BUG/MEDIUM: connection: Preserve flags when a conn is removed from an idle list
The commit 5e1b0e7bf ("BUG/MEDIUM: connection: Clear flags when a conn is
removed from an idle list") introduced a regression. CO_FL_SAFE_LIST and
CO_FL_IDLE_LIST flags are used when the connection is released to properly
decrement used/idle connection counters. if a connection is idle, these
flags must be preserved till the connection is really released. It may be
removed from the list but not immediately released. If these flags are lost
when it is finally released, the current number of used connections is
erroneously decremented. If means this counter may become negative and the
counters tracking the number of idle connecitons is not decremented,
suggesting a leak.
So, the above commit is reverted and instead we improve a bit the way to
detect an idle connection. The function conn_get_idle_flag() must now be
used to know if a connection is in an idle list. It returns the connection
flag corresponding to the idle list if the connection is idle
(CO_FL_SAFE_LIST or CO_FL_IDLE_LIST) or 0 otherwise. But if the connection
is scheduled to be removed, 0 is also returned, regardless the connection
flags.
This new function is used when the connection is temporarily removed from
the list to be used, mainly in muxes.
This patch should fix #2078 and #2057. It must be backported as far as 2.2.
(cherry picked from commit 3a7b539b124bccaa57478e0a5a6d66338594615a)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit a81a1e2aea0793aa624565a14cb7579b907f116a)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit a53fdaf7203e45f67c44d7e250cec36875ea8e01)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit c05ef082a02ff05090ddbfa6b187a3e3bc2504aa)
[cf: Context adjustement]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
diff --git a/include/haproxy/connection.h b/include/haproxy/connection.h
index 4b8b098..157387b 100644
--- a/include/haproxy/connection.h
+++ b/include/haproxy/connection.h
@@ -218,6 +218,16 @@
c->xprt->shutw(c, c->xprt_ctx, 0);
}
+/* Used to know if a connection is in an idle list. It returns connection flag
+ * corresponding to the idle list if the connection is idle (CO_FL_SAFE_LIST or
+ * CO_FL_IDLE_LIST) or 0 otherwise. Note that if the connection is scheduled to
+ * be removed, 0 is returned, regardless the connection flags.
+ */
+static inline unsigned int conn_get_idle_flag(const struct connection *conn)
+{
+ return (!MT_LIST_INLIST(&conn->toremove_list) ? conn->flags & CO_FL_LIST_MASK : 0);
+}
+
/* This is used at the end of the socket IOCB to possibly create the mux if it
* was not done yet, or wake it up if flags changed compared to old_flags or if
* need_wake insists on this. It returns <0 if the connection was destroyed and
@@ -266,7 +276,7 @@
((conn->flags ^ old_flags) & CO_FL_NOTIFY_DONE) ||
((old_flags & CO_FL_WAIT_XPRT) && !(conn->flags & CO_FL_WAIT_XPRT))) &&
conn->mux && conn->mux->wake) {
- uint conn_in_list = conn->flags & CO_FL_LIST_MASK;
+ uint conn_in_list = conn_get_idle_flag(conn);
struct server *srv = objt_server(conn->target);
if (conn_in_list) {
diff --git a/src/mux_fcgi.c b/src/mux_fcgi.c
index f84b0bf..5025163 100644
--- a/src/mux_fcgi.c
+++ b/src/mux_fcgi.c
@@ -3031,7 +3031,7 @@
conn = fconn->conn;
TRACE_POINT(FCGI_EV_FCONN_WAKE, conn);
- conn_in_list = conn->flags & CO_FL_LIST_MASK;
+ conn_in_list = conn_get_idle_flag(conn);
if (conn_in_list)
conn_delete_from_tree(&conn->hash_node->node);
@@ -3215,10 +3215,8 @@
/* We're about to destroy the connection, so make sure nobody attempts
* to steal it from us.
*/
- if (fconn->conn->flags & CO_FL_LIST_MASK) {
+ if (fconn->conn->flags & CO_FL_LIST_MASK)
conn_delete_from_tree(&fconn->conn->hash_node->node);
- fconn->conn->flags &= ~CO_FL_LIST_MASK;
- }
HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
}
diff --git a/src/mux_h1.c b/src/mux_h1.c
index e6a9392..739148e 100644
--- a/src/mux_h1.c
+++ b/src/mux_h1.c
@@ -2912,7 +2912,7 @@
/* Remove the connection from the list, to be sure nobody attempts
* to use it while we handle the I/O events
*/
- conn_in_list = conn->flags & CO_FL_LIST_MASK;
+ conn_in_list = conn_get_idle_flag(conn);
if (conn_in_list)
conn_delete_from_tree(&conn->hash_node->node);
@@ -3036,10 +3036,8 @@
/* We're about to destroy the connection, so make sure nobody attempts
* to steal it from us.
*/
- if (h1c->conn->flags & CO_FL_LIST_MASK) {
+ if (h1c->conn->flags & CO_FL_LIST_MASK)
conn_delete_from_tree(&h1c->conn->hash_node->node);
- h1c->conn->flags &= ~CO_FL_LIST_MASK;
- }
HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
}
diff --git a/src/mux_h2.c b/src/mux_h2.c
index 17446f1..4c8f565 100644
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -3977,11 +3977,10 @@
conn = h2c->conn;
TRACE_ENTER(H2_EV_H2C_WAKE, conn);
- conn_in_list = conn->flags & CO_FL_LIST_MASK;
-
/* Remove the connection from the list, to be sure nobody attempts
* to use it while we handle the I/O events
*/
+ conn_in_list = conn_get_idle_flag(conn);
if (conn_in_list)
conn_delete_from_tree(&conn->hash_node->node);
@@ -4096,7 +4095,6 @@
if (conn->flags & CO_FL_LIST_MASK) {
HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
conn_delete_from_tree(&conn->hash_node->node);
- conn->flags &= ~CO_FL_LIST_MASK;
HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
}
}
@@ -4105,7 +4103,6 @@
if (conn->flags & CO_FL_LIST_MASK) {
HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
conn_delete_from_tree(&conn->hash_node->node);
- conn->flags &= ~CO_FL_LIST_MASK;
HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
}
}
@@ -4186,10 +4183,8 @@
/* We're about to destroy the connection, so make sure nobody attempts
* to steal it from us.
*/
- if (h2c->conn->flags & CO_FL_LIST_MASK) {
+ if (h2c->conn->flags & CO_FL_LIST_MASK)
conn_delete_from_tree(&h2c->conn->hash_node->node);
- h2c->conn->flags &= ~CO_FL_LIST_MASK;
- }
HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
}
@@ -4242,7 +4237,6 @@
if (h2c->conn->flags & CO_FL_LIST_MASK) {
HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
conn_delete_from_tree(&h2c->conn->hash_node->node);
- h2c->conn->flags &= ~CO_FL_LIST_MASK;
HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
}
diff --git a/src/server.c b/src/server.c
index 93481dc..df67e1d 100644
--- a/src/server.c
+++ b/src/server.c
@@ -5463,7 +5463,6 @@
hash_node = ebmb_entry(node, struct conn_hash_node, node);
eb_delete(node);
- hash_node->conn->flags &= ~CO_FL_LIST_MASK;
MT_LIST_APPEND(toremove_list, &hash_node->conn->toremove_list);
i++;
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 8752e8d..e69812f 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -5948,7 +5948,7 @@
return NULL;
}
conn = ctx->conn;
- conn_in_list = conn->flags & CO_FL_LIST_MASK;
+ conn_in_list = conn_get_idle_flag(conn);
if (conn_in_list)
conn_delete_from_tree(&conn->hash_node->node);
HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);