BUG/MEDIUM: server: extend thread-isolate over much of CLI 'add server'

Some config parsing handlers were designed to be run at startup on a
single-thread. When executing at runtime for dynamic servers,
thread-safety is not guaranteed. This is the case for example in
srv_parse_id which manipulates backend used_ids tree.

One solution could be to add locks but it might be tricky to found all
affected functions and it can be an easy source of deadlock. The other
solution which has been chosen is to use thread-isolation over almost
all of the cli_parse_add_server CLI handler.

For now this solution is sufficient. If some users make heavy use of the
'add server', hurting the overall performance, it will be necessary to
design a much thinner solution.

This must be backported up to 2.4.

diff --git a/src/server.c b/src/server.c
index 57b3feecb6683a6cf2126bbe2bd99327fca1afe3..14f54857d579197f23b56b98d5b7db87efc0ef9e 100644 (file)
--- a/src/server.c
+++ b/src/server.c
@@ -4349,6 +4349,14 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
                return 1;
        }
 
+       /* At this point, some operations might not be thread-safe anymore. This
+        * might be the case for parsing handlers which were designed to run
+        * only at the starting stage on single-thread mode.
+        *
+        * Activate thread isolation to ensure thread-safety.
+        */
+       thread_isolate();
+
        args[1] = sv_name;
        errcode = _srv_parse_init(&srv, args, &argc, be, parse_flags);
        if (errcode)
@@ -4412,15 +4420,12 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
                goto out;
        }
 
-       /* Attach the server to the end of the proxy linked list. The proxy
-        * servers list is currently not protected by a lock, so this requires
-        * thread_isolate/release.
+       /* Attach the server to the end of the proxy linked list. Note that this
+        * operation is not thread-safe so this is executed under thread
+        * isolation.
         *
-        * If a server with the same name is found, reject the new one. This
-        * operation requires thread-safety and thus cannot be executed at the
-        * beginning without having server allocation under locks/isolation.
+        * If a server with the same name is found, reject the new one.
         */
-       thread_isolate();
 
        /* TODO use a double-linked list for px->srv */
        if (be->srv) {
@@ -4429,7 +4434,6 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
                while (1) {
                        /* check for duplicate server */
                        if (!strcmp(srv->id, next->id)) {
-                               thread_release();
                                ha_alert("Already exists a server with the same name in backend.\n");
                                goto out;
                        }
@@ -4455,6 +4459,8 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
        return 0;
 
 out:
+       thread_release();
+
        if (!usermsgs_empty())
                cli_err(appctx, usermsgs_str());