BUG/MINOR: haproxy/threads: close a possible race in soft-stop detection

Commit 4b3f27b ("BUG/MINOR: haproxy/threads: try to make all threads
leave together") improved the soft-stop synchronization but it left a
small race open because it looks at tasks_run_queue, which can drop
to zero then back to one while another thread picks the task from the
run queue to insert it into the tasklet_list. The risk is very low but
not null. In addition the condition didn't consider the possible presence
of signals in the queue.

This patch moves the stopping detection just after the "wake" calculation
which already takes care of the various queues' sizes and signals. It
avoids needlessly duplicating these tests.

The bug was discovered during a code review but will probably never be
observed. This fix may be backported to 2.1 and 2.0 along with the commit
above.

diff --git a/src/haproxy.c b/src/haproxy.c
index 4feee95..ca0ab32 100644 (file)
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -2806,14 +2806,6 @@ void run_poll_loop()
                if (tid == 0)
                        signal_process_queue();
 
-               if (stopping && tasks_run_queue == 0)
-                       _HA_ATOMIC_OR(&stopping_thread_mask, tid_bit);
-
-               /* stop when there's nothing left to do */
-               if ((jobs - unstoppable_jobs) == 0 && tasks_run_queue == 0 &&
-                   (stopping_thread_mask & all_threads_mask) == all_threads_mask)
-                       break;
-
                /* also stop  if we failed to cleanly stop all tasks */
                if (killed > 1)
                        break;
@@ -2834,6 +2826,16 @@ void run_poll_loop()
                                wake = 0;
                }
 
+               if (!wake) {
+                       if (stopping)
+                               _HA_ATOMIC_OR(&stopping_thread_mask, tid_bit);
+
+                       /* stop when there's nothing left to do */
+                       if ((jobs - unstoppable_jobs) == 0 &&
+                           (stopping_thread_mask & all_threads_mask) == all_threads_mask)
+                               break;
+               }
+
                /* If we have to sleep, measure how long */
                next = wake ? TICK_ETERNITY : next_timer_expiry();