BUG/MINOR: signals/poller: ensure wakeup from signals
Add self-wake in signal_handler() to fix a race condition with a signal
coming in between checking signal_queue_len and entering polling sleep.
The changes in commit 43c891dda ("BUG/MINOR: signals/poller: set the
poller timeout to 0 when there are signals") were insufficient.
Move the signal_queue_len check from the poll implementations to
run_poll_loop() to keep that logic in one place.
The poll loops are terminated either by the parameter wake being set or
wake up due to a write to their poller_wr_pipe by wake_thread() in
signal_handler().
This fixes issue #1841.
Must be backported in every stable version.
(cherry picked from commit eea152ee68e82eae49ae188cd1b1fbbf63dc6913)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 4a1f703e9ac18c98248e4e157a000cd270989cc2)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 3fbe970d9ab4ed211687101fb61ca49d809a2f58)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
diff --git a/src/ev_epoll.c b/src/ev_epoll.c
index 36fdc3a..041d6d8 100644
--- a/src/ev_epoll.c
+++ b/src/ev_epoll.c
@@ -185,10 +185,8 @@
thread_harmless_now();
- /* Now let's wait for polled events.
- * Check if the signal queue is not empty in case we received a signal
- * before entering the loop, so we don't wait MAX_DELAY_MS for nothing */
- wait_time = (wake | signal_queue_len) ? 0 : compute_poll_timeout(exp);
+ /* Now let's wait for polled events. */
+ wait_time = wake ? 0 : compute_poll_timeout(exp);
tv_entering_poll();
activity_count_runtime();
do {
@@ -203,7 +201,7 @@
}
if (timeout || !wait_time)
break;
- if (signal_queue_len || wake)
+ if (wake)
break;
if (tick_isset(exp) && tick_is_expired(exp, now_ms))
break;
diff --git a/src/ev_evports.c b/src/ev_evports.c
index e80b9c2..e012e00 100644
--- a/src/ev_evports.c
+++ b/src/ev_evports.c
@@ -153,10 +153,8 @@
thread_harmless_now();
- /* Now let's wait for polled events.
- * Check if the signal queue is not empty in case we received a signal
- * before entering the loop, so we don't wait MAX_DELAY_MS for nothing */
- wait_time = (wake | signal_queue_len) ? 0 : compute_poll_timeout(exp);
+ /* Now let's wait for polled events. */
+ wait_time = wake ? 0 : compute_poll_timeout(exp);
tv_entering_poll();
activity_count_runtime();
@@ -195,7 +193,7 @@
break;
if (timeout || !wait_time)
break;
- if (signal_queue_len || wake)
+ if (wake)
break;
if (tick_isset(exp) && tick_is_expired(exp, now_ms))
break;
diff --git a/src/ev_kqueue.c b/src/ev_kqueue.c
index db7c9cd..0d81d67 100644
--- a/src/ev_kqueue.c
+++ b/src/ev_kqueue.c
@@ -141,10 +141,8 @@
}
fd_nbupdt = 0;
- /* Now let's wait for polled events.
- * Check if the signal queue is not empty in case we received a signal
- * before entering the loop, so we don't wait MAX_DELAY_MS for nothing */
- wait_time = (wake | signal_queue_len) ? 0 : compute_poll_timeout(exp);
+ /* Now let's wait for polled events. */
+ wait_time = wake ? 0 : compute_poll_timeout(exp);
fd = global.tune.maxpollevents;
tv_entering_poll();
activity_count_runtime();
@@ -169,7 +167,7 @@
}
if (timeout || !wait_time)
break;
- if (signal_queue_len || wake)
+ if (wake)
break;
if (tick_isset(exp) && tick_is_expired(exp, now_ms))
break;
diff --git a/src/ev_poll.c b/src/ev_poll.c
index da675eb..4d136a0 100644
--- a/src/ev_poll.c
+++ b/src/ev_poll.c
@@ -199,10 +199,8 @@
}
}
- /* Now let's wait for polled events.
- * Check if the signal queue is not empty in case we received a signal
- * before entering the loop, so we don't wait MAX_DELAY_MS for nothing */
- wait_time = (wake | signal_queue_len) ? 0 : compute_poll_timeout(exp);
+ /* Now let's wait for polled events. */
+ wait_time = wake ? 0 : compute_poll_timeout(exp);
tv_entering_poll();
activity_count_runtime();
status = poll(poll_events, nbfd, wait_time);
diff --git a/src/haproxy.c b/src/haproxy.c
index 4ee443e..a114deb 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -2633,7 +2633,7 @@
if (killed > 1)
break;
- /* expire immediately if events are pending */
+ /* expire immediately if events or signals are pending */
wake = 1;
if (thread_has_tasks())
activity[tid].wake_tasks++;
@@ -2643,6 +2643,10 @@
if (thread_has_tasks()) {
activity[tid].wake_tasks++;
_HA_ATOMIC_AND(&sleeping_thread_mask, ~tid_bit);
+ } else if (signal_queue_len) {
+ /* this check is required to avoid
+ * a race with wakeup on signals using wake_threads() */
+ _HA_ATOMIC_AND(&sleeping_thread_mask, ~tid_bit);
} else
wake = 0;
}
diff --git a/src/signal.c b/src/signal.c
index 5ec7142..3d7a9a8 100644
--- a/src/signal.c
+++ b/src/signal.c
@@ -56,6 +56,9 @@
signal_state[sig].count++;
if (sig)
signal(sig, signal_handler); /* re-arm signal */
+
+ /* If the thread is TH_FL_SLEEPING we need to wake it */
+ wake_thread(tid);
}
/* Call handlers of all pending signals and clear counts and queue length. The