From: William Lallemand <wlallemand@haproxy.com>
Date: Thu, 7 Jun 2018 07:46:01 +0000 (+0200)
Subject: BUG/MEDIUM: threads: handle signal queue only in thread 0
X-Git-Tag: v1.9-dev1~208
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=1aab50bb4aaf59f0dd6f0e98ccbf023209f92a69;p=thirdparty%2Fhaproxy.git

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.

This patch must be backported in 1.8.
---

diff --git a/src/haproxy.c b/src/haproxy.c
index 766758f628..96b8abcf9b 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -2402,8 +2402,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();
@@ -2418,7 +2420,7 @@ static void run_poll_loop()
 			activity[tid].wake_cache++;
 		else if (active_tasks_mask & tid_bit)
 			activity[tid].wake_tasks++;
-		else if (signal_queue_len)
+		else if (signal_queue_len && tid == 0)
 			activity[tid].wake_signal++;
 		else
 			exp = next;
@@ -3008,6 +3010,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);
 
@@ -3015,6 +3018,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++)
@@ -3048,6 +3060,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 a0975910b1..f1f6821883 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