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.
(cherry picked from commit 31ddd76fef720c35c22acc39016feea5604e5b39)
[wt: ctx adjustments around error messages]
Signed-off-by: Willy Tarreau <w@1wt.eu>
diff --git a/src/server.c b/src/server.c
index f04a055..0280edd 100644
--- a/src/server.c
+++ b/src/server.c
@@ -4354,6 +4354,14 @@
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, &errmsg);
if (errcode) {
@@ -4418,15 +4426,12 @@
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) {
@@ -4435,7 +4440,6 @@
while (1) {
/* check for duplicate server */
if (!strcmp(srv->id, next->id)) {
- thread_release();
cli_err(appctx, "Already exists a server with the same name in backend.");
goto out;
}
@@ -4461,6 +4465,7 @@
return 0;
out:
+ thread_release();
if (srv)
free_server(srv);
return 1;