mirrors/haproxy
1
0
Fork 0
mirror of https://git.haproxy.org/git/haproxy.git/ synced 2025-09-22 14:21:25 +02:00
Code Issues Projects Releases Wiki Activity

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

Browse Source 
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.
This commit is contained in:
Amaury Denoyelle 2021-06-10 15:26:44 +02:00
parent 077c6b8d29
commit 31ddd76fef
1 changed files with 14 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
src/server.c
View File

@ -4349,6 +4349,14 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
return 1; 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; args[1] = sv_name;
errcode = _srv_parse_init(&srv, args, &argc, be, parse_flags); errcode = _srv_parse_init(&srv, args, &argc, be, parse_flags);
if (errcode) if (errcode)
@ -4412,15 +4420,12 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
goto out; goto out;
} }
/* Attach the server to the end of the proxy linked list. The proxy /* Attach the server to the end of the proxy linked list. Note that this
* servers list is currently not protected by a lock, so this requires * operation is not thread-safe so this is executed under thread
* thread_isolate/release. * isolation.
* *
* If a server with the same name is found, reject the new one. This * If a server with the same name is found, reject the new one.
* operation requires thread-safety and thus cannot be executed at the
* beginning without having server allocation under locks/isolation.
*/ */
thread_isolate();
/* TODO use a double-linked list for px->srv */ /* TODO use a double-linked list for px->srv */
if (be->srv) { if (be->srv) {
@ -4429,7 +4434,6 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
while (1) { while (1) {
/* check for duplicate server */ /* check for duplicate server */
if (!strcmp(srv->id, next->id)) { if (!strcmp(srv->id, next->id)) {
thread_release();
ha_alert("Already exists a server with the same name in backend.\n"); ha_alert("Already exists a server with the same name in backend.\n");
goto out; goto out;
} }
@ -4455,6 +4459,8 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
return 0; return 0;
out: out:
thread_release();
if (!usermsgs_empty()) if (!usermsgs_empty())
cli_err(appctx, usermsgs_str()); cli_err(appctx, usermsgs_str());