Skip to content

Commit 32f0486

Browse files
HeikoKlareclaude
andcommitted
Fix race condition in Synchronizer.addLast() causing missed wakeThread()
calls The old implementation checked messages.isEmpty() before calling messages.add(), creating a window where the UI thread could drain the queue and enter sleep() between the isEmpty() check (which saw items) and the add() call. Because wake was set to false from the stale isEmpty() result, wakeThread() was never called and the Display would block indefinitely despite having a pending message. Fix by adding the lock first, then using peek() == lock (reference equality on ConcurrentLinkedQueue) to detect that our item landed at the head, which means the queue was empty — wake the thread only then. Adds a regression test with concurrent producers to validate the fix. Fixes #3151 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 2cd0808 commit 32f0486

3 files changed

Lines changed: 120 additions & 2 deletions

File tree

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/Synchronizer.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,9 +74,8 @@ void moveAllEventsTo (Synchronizer toReceiveTheEvents) {
7474

7575

7676
void addLast (RunnableLock lock) {
77-
boolean wake = messages.isEmpty();
7877
messages.add(lock);
79-
if (wake) display.wakeThread ();
78+
if (messages.peek() == lock) display.wakeThread ();
8079
}
8180

8281
/**

tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/AllWidgetTests.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@
7373
Test_org_eclipse_swt_widgets_Shell.class, //
7474
Test_org_eclipse_swt_widgets_Slider.class, //
7575
Test_org_eclipse_swt_widgets_Spinner.class, //
76+
Test_org_eclipse_swt_widgets_Synchronizer.class, //
7677
Test_org_eclipse_swt_widgets_TabFolder.class, //
7778
Test_org_eclipse_swt_widgets_TabItem.class, //
7879
Test_org_eclipse_swt_widgets_Table.class, //
Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
/*******************************************************************************
2+
* Copyright (c) 2026 Contributors to the Eclipse Project
3+
*
4+
* This program and the accompanying materials
5+
* are made available under the terms of the Eclipse Public License 2.0
6+
* which accompanies this distribution, and is available at
7+
* https://www.eclipse.org/legal/epl-2.0/
8+
*
9+
* SPDX-License-Identifier: EPL-2.0
10+
*******************************************************************************/
11+
package org.eclipse.swt.tests.junit;
12+
13+
import static org.junit.jupiter.api.Assertions.assertEquals;
14+
import static org.junit.jupiter.api.Assertions.assertFalse;
15+
16+
import java.util.concurrent.atomic.AtomicBoolean;
17+
import java.util.concurrent.atomic.AtomicInteger;
18+
19+
import org.eclipse.swt.widgets.Display;
20+
import org.junit.jupiter.api.Test;
21+
import org.junit.jupiter.api.Timeout;
22+
23+
/**
24+
* Automated Test Suite for class org.eclipse.swt.widgets.Synchronizer
25+
*
26+
* @see org.eclipse.swt.widgets.Synchronizer
27+
*/
28+
public class Test_org_eclipse_swt_widgets_Synchronizer {
29+
30+
private static final int TIMEOUT_MS = 20_000;
31+
32+
/**
33+
* Regression test for
34+
* https://github.com/eclipse-platform/eclipse.platform.swt/issues/3151
35+
*
36+
* <p>
37+
* Verifies that every task posted via {@code asyncExec()} from a non-UI thread
38+
* is eventually executed, even when posted concurrently while the UI thread is
39+
* draining the message queue.
40+
*
41+
* <p>
42+
* The old {@code Synchronizer.addLast()} implementation had this race:
43+
* <ol>
44+
* <li>Producer thread: {@code messages.isEmpty()} → {@code false} (queue has
45+
* items), so {@code wake = false}</li>
46+
* <li>UI thread: drains all remaining items; calls {@code sleep()} →
47+
* {@code OS.WaitMessage()} / blocks</li>
48+
* <li>Producer thread: {@code messages.add(lock)}</li>
49+
* <li>Producer thread: {@code if (wake)} → {@code false} → {@code wakeThread()}
50+
* never called → Display sleeps forever despite a pending message</li>
51+
* </ol>
52+
*
53+
* <p>
54+
* Using multiple concurrent producers maximises the probability of hitting the
55+
* race window: one producer's task appears in the queue when another producer
56+
* evaluates {@code isEmpty()}, then that task gets consumed and the UI sleeps
57+
* before the second producer's {@code add()} completes.
58+
*
59+
* <p>
60+
* A {@code timerExec} sentinel bounds the test duration so a hung Display
61+
* produces a test failure rather than an infinite hang.
62+
*/
63+
@Test
64+
@Timeout(TIMEOUT_MS)
65+
public void test_asyncExec_allTasksComplete_noMissedWakeup() throws InterruptedException {
66+
final int PRODUCERS = 4;
67+
final int TASKS_PER_PRODUCER = 1_000;
68+
final int TOTAL_TASKS = PRODUCERS * TASKS_PER_PRODUCER;
69+
final int TIMEOUT_MS = 15_000;
70+
71+
Display display = Display.getDefault();
72+
try {
73+
AtomicInteger completedCount = new AtomicInteger();
74+
AtomicBoolean timedOut = new AtomicBoolean(false);
75+
76+
// Safety net: if the Display gets stuck due to a missed wakeThread() call,
77+
// the timer fires on the UI thread and lets the event loop exit cleanly so
78+
// the assertion below produces a clear failure instead of hanging CI.
79+
display.timerExec(TIMEOUT_MS * 3/4, () -> timedOut.set(true));
80+
81+
// Multiple concurrent producers increase the chance that one producer's
82+
// isEmpty() check sees another's task, which is then drained before the
83+
// first producer's add() completes – the exact sequence that triggers the bug.
84+
Thread[] producers = new Thread[PRODUCERS];
85+
for (int p = 0; p < PRODUCERS; p++) {
86+
producers[p] = new Thread(() -> {
87+
for (int i = 0; i < TASKS_PER_PRODUCER; i++) {
88+
if (timedOut.get()) {
89+
return;
90+
}
91+
display.asyncExec(completedCount::incrementAndGet);
92+
}
93+
}, "asyncExec-producer-" + p);
94+
producers[p].start();
95+
}
96+
97+
while (completedCount.get() < TOTAL_TASKS && !timedOut.get()) {
98+
if (!display.readAndDispatch()) {
99+
display.sleep();
100+
}
101+
}
102+
103+
for (Thread producer : producers) {
104+
producer.join(1_000);
105+
}
106+
107+
assertFalse(timedOut.get(),
108+
"Display did not process all asyncExec tasks within " + TIMEOUT_MS + " ms " + "(completed "
109+
+ completedCount.get() + "/" + TOTAL_TASKS + "). "
110+
+ "Likely a missed wakeThread() call in Synchronizer.addLast() "
111+
+ "– regression of https://github.com/eclipse-platform/eclipse.platform.swt/issues/3151");
112+
assertEquals(TOTAL_TASKS, completedCount.get(), "Not all asyncExec tasks were executed.");
113+
} finally {
114+
display.dispose();
115+
}
116+
}
117+
118+
}

0 commit comments

Comments
 (0)