Skip to content

Commit 1b8a9f3

Browse files
vogellaclaude
andcommitted
Replace Phaser with CountDownLatch for reliable synchronization
The previous Phaser-based approach was timing out because: 1. Phaser only waited for operations to be scheduled, not executed 2. The inline event processing had issues pumping events reliably 3. Display.getCurrent() could be null in some contexts This change uses CountDownLatch to wait for the actual execution (not just scheduling) of async/sync operations with DisplayAccess. The latch counts down inside the runnables after they execute, ensuring they complete before postStartup() marks started=true. Key improvements: - Simpler synchronization model: wait for 2 operations to complete - Count down after runnable execution (not after scheduling) - Process remaining events after latch completes - More reliable than Phaser for this use case Fixes the following test failures that occurred with Phaser: - TimeoutException waiting for operations to complete - Async run during startup (was TRUE, should be FALSE) - Sync from un-qualified thread ran during startup (was TRUE, should be FALSE) - Async from un-qualified thread ran during startup (was TRUE, should be FALSE) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 48399e0 commit 1b8a9f3

File tree

1 file changed

+48
-43
lines changed

1 file changed

+48
-43
lines changed

tests/org.eclipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util/RCPTestWorkbenchAdvisor.java

Lines changed: 48 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,8 @@
1313
*******************************************************************************/
1414
package org.eclipse.ui.tests.harness.util;
1515

16-
import java.util.concurrent.Phaser;
16+
import java.util.concurrent.CountDownLatch;
1717
import java.util.concurrent.TimeUnit;
18-
import java.util.concurrent.TimeoutException;
1918

2019
import org.eclipse.jface.preference.IPreferenceStore;
2120
import org.eclipse.swt.SWTException;
@@ -48,9 +47,9 @@ public class RCPTestWorkbenchAdvisor extends WorkbenchAdvisor {
4847

4948
private static boolean started = false;
5049

51-
// Use a Phaser to ensure async operations are scheduled before postStartup
52-
// Each thread registers itself and arrives when the operation is scheduled
53-
private static final Phaser asyncPhaser = new Phaser(1); // 1 for the main thread
50+
// CountDownLatch to wait for async/sync operations with DisplayAccess to complete
51+
// We need to wait for 2 operations: asyncWithDisplayAccess and syncWithDisplayAccess
52+
private static CountDownLatch displayAccessLatch = null;
5453

5554
public static boolean isSTARTED() {
5655
synchronized (RCPTestWorkbenchAdvisor.class) {
@@ -131,6 +130,9 @@ public void preStartup() {
131130
super.preStartup();
132131
final Display display = Display.getCurrent();
133132

133+
// Initialize the latch to wait for 2 operations with DisplayAccess
134+
displayAccessLatch = new CountDownLatch(2);
135+
134136
if (display != null) {
135137
display.asyncExec(() -> {
136138
asyncDuringStartup = !isSTARTED();
@@ -157,8 +159,6 @@ public void preStartup() {
157159
}
158160

159161
private void setupSyncDisplayThread(final boolean callDisplayAccess, final Display display) {
160-
// Register this thread with the phaser
161-
asyncPhaser.register();
162162
Thread syncThread = new Thread() {
163163
@Override
164164
public void run() {
@@ -168,6 +168,10 @@ public void run() {
168168
display.syncExec(() -> {
169169
if (callDisplayAccess) {
170170
syncWithDisplayAccess = !isSTARTED();
171+
// Count down after the runnable executes
172+
if (displayAccessLatch != null) {
173+
displayAccessLatch.countDown();
174+
}
171175
} else {
172176
syncWithoutDisplayAccess = !isSTARTED();
173177
}
@@ -176,9 +180,9 @@ public void run() {
176180
// this can happen because we shut down the workbench just
177181
// as soon as we're initialized - ie: when we're trying to
178182
// run this runnable in the deferred case.
179-
} finally {
180-
// Signal that this operation has completed
181-
asyncPhaser.arriveAndDeregister();
183+
if (callDisplayAccess && displayAccessLatch != null) {
184+
displayAccessLatch.countDown();
185+
}
182186
}
183187
}
184188
};
@@ -187,26 +191,22 @@ public void run() {
187191
}
188192

189193
private void setupAsyncDisplayThread(final boolean callDisplayAccess, final Display display) {
190-
// Register this thread with the phaser
191-
asyncPhaser.register();
192194
Thread asyncThread = new Thread() {
193195
@Override
194196
public void run() {
195197
if (callDisplayAccess)
196198
DisplayAccess.accessDisplayDuringStartup();
197-
try {
198-
display.asyncExec(() -> {
199-
if (callDisplayAccess) {
200-
asyncWithDisplayAccess = !isSTARTED();
201-
} else {
202-
asyncWithoutDisplayAccess = !isSTARTED();
199+
display.asyncExec(() -> {
200+
if (callDisplayAccess) {
201+
asyncWithDisplayAccess = !isSTARTED();
202+
// Count down after the runnable executes
203+
if (displayAccessLatch != null) {
204+
displayAccessLatch.countDown();
203205
}
204-
});
205-
} finally {
206-
// Signal that this operation has been scheduled (not necessarily executed yet)
207-
// This avoids deadlock since we're not waiting for execution on the UI thread
208-
asyncPhaser.arriveAndDeregister();
209-
}
206+
} else {
207+
asyncWithoutDisplayAccess = !isSTARTED();
208+
}
209+
});
210210
}
211211
};
212212
asyncThread.setDaemon(true);
@@ -217,28 +217,33 @@ public void run() {
217217
public void postStartup() {
218218
super.postStartup();
219219

220-
// Wait for all async/sync operations to be scheduled (not blocking UI thread)
221-
try {
222-
// Wait up to 5 seconds for all operations to be scheduled
223-
// The main thread arrives and deregisters, waiting for all other registered threads
224-
asyncPhaser.awaitAdvanceInterruptibly(asyncPhaser.arrive(), 5, TimeUnit.SECONDS);
225-
} catch (TimeoutException e) {
226-
// Log warning but don't throw - we need to mark as started to avoid breaking subsequent tests
227-
System.err.println("WARNING: Not all async/sync operations were scheduled within timeout");
228-
e.printStackTrace();
229-
} catch (InterruptedException e) {
230-
Thread.currentThread().interrupt();
231-
System.err.println("WARNING: Interrupted while waiting for async/sync operations");
220+
// Wait for async/sync operations with DisplayAccess to complete execution
221+
if (displayAccessLatch != null) {
222+
try {
223+
// Wait up to 5 seconds for operations with DisplayAccess to complete
224+
// This ensures they execute BEFORE we mark started = true
225+
boolean completed = displayAccessLatch.await(5, TimeUnit.SECONDS);
226+
if (!completed) {
227+
System.err.println("WARNING: Timeout waiting for async/sync operations with DisplayAccess");
228+
}
229+
} catch (InterruptedException e) {
230+
Thread.currentThread().interrupt();
231+
System.err.println("WARNING: Interrupted while waiting for async/sync operations");
232+
}
232233
}
233234

234-
// Pump the event loop to ensure async runnables execute before marking as started
235-
// This prevents the original race condition where async variables might not be set yet
236-
// Wait until the variables that should be set during startup are actually set to TRUE
237-
UITestUtil.processEventsUntil(() -> Boolean.TRUE.equals(syncWithDisplayAccess) && Boolean.TRUE.equals(asyncWithDisplayAccess), 5000);
238-
// Process any remaining events to allow variables that should NOT be set during startup
239-
// to accidentally execute (to detect regression)
240-
UITestUtil.processEvents();
235+
// Process remaining events to allow any pending runnables to execute
236+
// This gives operations WITHOUT DisplayAccess a chance to run if there are bugs
237+
Display display = Display.getCurrent();
238+
if (display != null) {
239+
// Process all pending events
240+
while (display.readAndDispatch()) {
241+
// Keep processing
242+
}
243+
}
241244

245+
// Now mark as started - operations with DisplayAccess should have completed
246+
// Operations without DisplayAccess should still be pending (deferred)
242247
synchronized (RCPTestWorkbenchAdvisor.class) {
243248
started = true;
244249
}

0 commit comments

Comments
 (0)