Hi guys, Sorry for the late reply, I manage to reproduce and fix what seams to be the bug. The signal management was not handled correctly with threads.
Could you try those patches and see if it fixes the problem? Thanks. -- William Lallemand
>From d695242fb260538bd8db323715d627c4a9deacc7 Mon Sep 17 00:00:00 2001 From: William Lallemand <[email protected]> Date: Thu, 7 Jun 2018 09:46:01 +0200 Subject: [PATCH 1/3] BUG/MEDIUM: threads: handle signal queue only in thread 0 Signals were handled in all threads which caused some signals to be lost from time to time. To avoid complicated lock system (threads+signals), we prefer handling the signals in one thread avoiding concurrent access. The side effect of this bug was that some process were not leaving from time to time during a reload. --- src/haproxy.c | 21 ++++++++++++++++++--- src/signal.c | 8 -------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/haproxy.c b/src/haproxy.c index 4628d8296..e984ca573 100644 --- a/src/haproxy.c +++ b/src/haproxy.c @@ -2398,8 +2398,10 @@ static void run_poll_loop() /* Process a few tasks */ process_runnable_tasks(); - /* check if we caught some signals and process them */ - signal_process_queue(); + /* check if we caught some signals and process them in the + first thread */ + if (tid == 0) + signal_process_queue(); /* Check if we can expire some tasks */ next = wake_expired_tasks(); @@ -2416,7 +2418,7 @@ static void run_poll_loop() activity[tid].wake_tasks++; else if (active_applets_mask & tid_bit) activity[tid].wake_applets++; - else if (signal_queue_len) + else if (signal_queue_len && tid == 0) activity[tid].wake_signal++; else exp = next; @@ -3006,6 +3008,7 @@ int main(int argc, char **argv) unsigned int *tids = calloc(global.nbthread, sizeof(unsigned int)); pthread_t *threads = calloc(global.nbthread, sizeof(pthread_t)); int i; + sigset_t blocked_sig, old_sig; THREAD_SYNC_INIT((1UL << global.nbthread) - 1); @@ -3013,6 +3016,15 @@ int main(int argc, char **argv) for (i = 0; i < global.nbthread; i++) tids[i] = i; + /* ensure the signals will be blocked in every thread */ + sigfillset(&blocked_sig); + sigdelset(&blocked_sig, SIGPROF); + sigdelset(&blocked_sig, SIGBUS); + sigdelset(&blocked_sig, SIGFPE); + sigdelset(&blocked_sig, SIGILL); + sigdelset(&blocked_sig, SIGSEGV); + pthread_sigmask(SIG_SETMASK, &blocked_sig, &old_sig); + /* Create nbthread-1 thread. The first thread is the current process */ threads[0] = pthread_self(); for (i = 1; i < global.nbthread; i++) @@ -3046,6 +3058,9 @@ int main(int argc, char **argv) } #endif /* !USE_CPU_AFFINITY */ + /* when multithreading we need to let only the thread 0 handle the signals */ + pthread_sigmask(SIG_SETMASK, &old_sig, NULL); + /* Finally, start the poll loop for the first thread */ run_thread_poll_loop(&tids[0]); diff --git a/src/signal.c b/src/signal.c index a0975910b..f1f682188 100644 --- a/src/signal.c +++ b/src/signal.c @@ -31,7 +31,6 @@ struct pool_head *pool_head_sig_handlers = NULL; sigset_t blocked_sig; int signal_pending = 0; /* non-zero if t least one signal remains unprocessed */ -__decl_hathreads(HA_SPINLOCK_T signals_lock); /* Common signal handler, used by all signals. Received signals are queued. * Signal number zero has a specific status, as it cannot be delivered by the @@ -71,9 +70,6 @@ void __signal_process_queue() struct signal_descriptor *desc; sigset_t old_sig; - if (HA_SPIN_TRYLOCK(SIGNALS_LOCK, &signals_lock)) - return; - /* block signal delivery during processing */ sigprocmask(SIG_SETMASK, &blocked_sig, &old_sig); @@ -100,7 +96,6 @@ void __signal_process_queue() /* restore signal delivery */ sigprocmask(SIG_SETMASK, &old_sig, NULL); - HA_SPIN_UNLOCK(SIGNALS_LOCK, &signals_lock); } /* perform minimal intializations, report 0 in case of error, 1 if OK. */ @@ -112,8 +107,6 @@ int signal_init() memset(signal_queue, 0, sizeof(signal_queue)); memset(signal_state, 0, sizeof(signal_state)); - HA_SPIN_INIT(&signals_lock); - /* Ensure signals are not blocked. Some shells or service managers may * accidently block all of our signals unfortunately, causing lots of * zombie processes to remain in the background during reloads. @@ -148,7 +141,6 @@ void deinit_signals() pool_free(pool_head_sig_handlers, sh); } } - HA_SPIN_DESTROY(&signals_lock); } /* Register a function and an integer argument on a signal. A pointer to the -- 2.16.1
>From 1501eddeb506897126d0d3d60a36ca780b24ffdf Mon Sep 17 00:00:00 2001 From: William Lallemand <[email protected]> Date: Thu, 7 Jun 2018 09:49:04 +0200 Subject: [PATCH 2/3] BUG/MINOR: don't ignore SIG{BUS,FPE,ILL,SEGV} during signal processing --- src/signal.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/signal.c b/src/signal.c index f1f682188..0dadd762c 100644 --- a/src/signal.c +++ b/src/signal.c @@ -120,6 +120,14 @@ int signal_init() sigfillset(&blocked_sig); sigdelset(&blocked_sig, SIGPROF); + /* man sigprocmask: If SIGBUS, SIGFPE, SIGILL, or SIGSEGV are + generated while they are blocked, the result is undefined, unless + the signal was generated by kill(2), + sigqueue(3), or raise(3) */ + sigdelset(&blocked_sig, SIGBUS); + sigdelset(&blocked_sig, SIGFPE); + sigdelset(&blocked_sig, SIGILL); + sigdelset(&blocked_sig, SIGSEGV); for (sig = 0; sig < MAX_SIGNAL; sig++) LIST_INIT(&signal_state[sig].handlers); -- 2.16.1
>From 73423f636d906e7815589f80088084cb7ca589b1 Mon Sep 17 00:00:00 2001 From: William Lallemand <[email protected]> Date: Thu, 7 Jun 2018 11:23:40 +0200 Subject: [PATCH 3/3] BUG/MINOR: signals: sigprocmask is unspecified in a multithread process Replace sigprocmask by pthread_sigmask. --- src/signal.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/signal.c b/src/signal.c index 0dadd762c..767d85c48 100644 --- a/src/signal.c +++ b/src/signal.c @@ -71,7 +71,11 @@ void __signal_process_queue() sigset_t old_sig; /* block signal delivery during processing */ +#ifdef USE_THREAD + pthread_sigmask(SIG_SETMASK, &blocked_sig, &old_sig); +#else sigprocmask(SIG_SETMASK, &blocked_sig, &old_sig); +#endif /* It is important that we scan the queue forwards so that we can * catch any signal that would have been queued by another signal @@ -95,7 +99,11 @@ void __signal_process_queue() signal_queue_len = 0; /* restore signal delivery */ +#ifdef USE_THREAD + pthread_sigmask(SIG_SETMASK, &old_sig, NULL); +#else sigprocmask(SIG_SETMASK, &old_sig, NULL); +#endif } /* perform minimal intializations, report 0 in case of error, 1 if OK. */ -- 2.16.1
