From: Amaury Denoyelle <adenoyelle@haproxy.com>
Date: Fri, 22 Sep 2023 14:28:35 +0000 (+0200)
Subject: MINOR: proto_reverse_connect: refactor preconnect failure
X-Git-Tag: v2.9-dev6~3
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=1f43fb71be0cda9132668de5cc01eafb02d371d4;p=thirdparty%2Fhaproxy.git

MINOR: proto_reverse_connect: refactor preconnect failure

When a connection is freed during preconnect before reversal, the error
must be notified to the listener to remove any connection reference and
rearm a new preconnect attempt. Currently, this can occur through 2 code
paths :
* conn_free() called directly by H2 mux
* error during conn_create_mux(). For this case, connection is flagged
  with CO_FL_ERROR and reverse_connect task is woken up. The process
  task handler is then responsible to call conn_free() for such
  connection.

Duplicated steps where done both in conn_free() and process task
handler. These are now removed. To facilitate code maintenance,
dedicated operation have been centralized in a new function
rev_notify_preconn_err() which is called by conn_free().
---

diff --git a/include/haproxy/proto_reverse_connect.h b/include/haproxy/proto_reverse_connect.h
index e40a0217a8..0c07356e09 100644
--- a/include/haproxy/proto_reverse_connect.h
+++ b/include/haproxy/proto_reverse_connect.h
@@ -16,4 +16,6 @@ int rev_set_affinity(struct connection *conn, int new_tid);
 
 int rev_accepting_conn(const struct receiver *rx);
 
+void rev_notify_preconn_err(struct listener *l);
+
 #endif /* _HAPROXY_PROTO_REVERSE_CONNECT_H */
diff --git a/src/connection.c b/src/connection.c
index 5f7226aaec..9c127c0fb1 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -25,6 +25,7 @@
 #include <haproxy/log-t.h>
 #include <haproxy/namespace.h>
 #include <haproxy/net_helper.h>
+#include <haproxy/proto_reverse_connect.h>
 #include <haproxy/proto_tcp.h>
 #include <haproxy/sample.h>
 #include <haproxy/sc_strm.h>
@@ -579,14 +580,7 @@ void conn_free(struct connection *conn)
 
 	if (conn_reverse_in_preconnect(conn)) {
 		struct listener *l = conn_active_reverse_listener(conn);
-
-		/* For the moment reverse connection are bound only on first thread. */
-		BUG_ON(tid != 0);
-		/* Receiver must reference a reverse connection as pending. */
-		BUG_ON(!l->rx.reverse_connect.pend_conn);
-		l->rx.reverse_connect.pend_conn = NULL;
-		l->rx.reverse_connect.task->expire = MS_TO_TICKS(now_ms + 1000);
-		task_queue(l->rx.reverse_connect.task);
+		rev_notify_preconn_err(l);
 	}
 
 	conn_force_unsubscribe(conn);
diff --git a/src/proto_reverse_connect.c b/src/proto_reverse_connect.c
index ef8fe21d48..cc6a6dfb0d 100644
--- a/src/proto_reverse_connect.c
+++ b/src/proto_reverse_connect.c
@@ -102,6 +102,26 @@ static struct connection *new_reverse_conn(struct listener *l, struct server *sr
 	return NULL;
 }
 
+/* Report that a connection used for preconnect on listener <l> is freed before
+ * reversal is completed. This is used to cleanup any reference to the
+ * connection and rearm a new preconnect attempt.
+ */
+void rev_notify_preconn_err(struct listener *l)
+{
+	/* For the moment reverse connection are bound only on first thread. */
+	BUG_ON(tid != 0);
+
+	/* Receiver must reference a reverse connection as pending. */
+	BUG_ON(!l->rx.reverse_connect.pend_conn);
+
+	/* Remove reference to the freed connection. */
+	l->rx.reverse_connect.pend_conn = NULL;
+
+	/* Rearm a new preconnect attempt. */
+	l->rx.reverse_connect.task->expire = MS_TO_TICKS(now_ms + 1000);
+	task_queue(l->rx.reverse_connect.task);
+}
+
 struct task *rev_process(struct task *task, void *ctx, unsigned int state)
 {
 	struct listener *l = ctx;
@@ -111,10 +131,9 @@ struct task *rev_process(struct task *task, void *ctx, unsigned int state)
 		if (conn->flags & CO_FL_ERROR) {
 			conn_full_close(conn);
 			conn_free(conn);
-			l->rx.reverse_connect.pend_conn = NULL;
 
-			/* Retry on 1s on error. */
-			l->rx.reverse_connect.task->expire = MS_TO_TICKS(now_ms + 1000);
+			/* conn_free() must report preconnect failure using rev_notify_preconn_err(). */
+			BUG_ON(l->rx.reverse_connect.pend_conn);
 		}
 		else {
 			/* Spurrious receiver task wake up when pend_conn is not ready/on error. */