Skip to content

Commit 13a3215

Browse files
vogellaclaude
andcommitted
Fix race condition in RCPTestWorkbenchAdvisor with Phaser synchronization
Replace CountDownLatch with Phaser to eliminate magic number and simplify event processing using UITestUtil helpers per review feedback. - Use Phaser instead of CountDownLatch for cleaner thread tracking - Simplify postStartup() event loop with UITestUtil.processEventsUntil() - Each thread self-registers with phaser, removing hardcoded count 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 parent 3caaf9b commit 13a3215

File tree

2 files changed

+55
-15
lines changed

2 files changed

+55
-15
lines changed

tests/org.eclipse.ui.tests.harness/META-INF/MANIFEST.MF

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
22
Bundle-ManifestVersion: 2
33
Bundle-Name: Harness Plug-in
44
Bundle-SymbolicName: org.eclipse.ui.tests.harness;singleton:=true
5-
Bundle-Version: 1.10.600.qualifier
5+
Bundle-Version: 1.10.700.qualifier
66
Eclipse-BundleShape: dir
77
Require-Bundle: org.eclipse.ui,
88
org.eclipse.core.runtime,

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

Lines changed: 54 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,10 @@
1313
*******************************************************************************/
1414
package org.eclipse.ui.tests.harness.util;
1515

16+
import java.util.concurrent.Phaser;
17+
import java.util.concurrent.TimeUnit;
18+
import java.util.concurrent.TimeoutException;
19+
1620
import org.eclipse.jface.preference.IPreferenceStore;
1721
import org.eclipse.swt.SWTException;
1822
import org.eclipse.swt.widgets.Display;
@@ -44,6 +48,10 @@ public class RCPTestWorkbenchAdvisor extends WorkbenchAdvisor {
4448

4549
private static boolean started = false;
4650

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
54+
4755
public static boolean isSTARTED() {
4856
synchronized (RCPTestWorkbenchAdvisor.class) {
4957
return started;
@@ -122,12 +130,10 @@ public void eventLoopIdle(final Display display) {
122130
public void preStartup() {
123131
super.preStartup();
124132
final Display display = Display.getCurrent();
133+
125134
if (display != null) {
126135
display.asyncExec(() -> {
127-
if (isSTARTED())
128-
asyncDuringStartup = Boolean.FALSE;
129-
else
130-
asyncDuringStartup = Boolean.TRUE;
136+
asyncDuringStartup = !isSTARTED();
131137
});
132138
}
133139

@@ -151,6 +157,8 @@ public void preStartup() {
151157
}
152158

153159
private void setupSyncDisplayThread(final boolean callDisplayAccess, final Display display) {
160+
// Register this thread with the phaser
161+
asyncPhaser.register();
154162
Thread syncThread = new Thread() {
155163
@Override
156164
public void run() {
@@ -160,15 +168,18 @@ public void run() {
160168
display.syncExec(() -> {
161169
synchronized (RCPTestWorkbenchAdvisor.class) {
162170
if (callDisplayAccess)
163-
syncWithDisplayAccess = !isSTARTED() ? Boolean.TRUE : Boolean.FALSE;
171+
syncWithDisplayAccess = !isSTARTED();
164172
else
165-
syncWithoutDisplayAccess = !isSTARTED() ? Boolean.TRUE : Boolean.FALSE;
173+
syncWithoutDisplayAccess = !isSTARTED();
166174
}
167175
});
168176
} catch (SWTException e) {
169177
// this can happen because we shut down the workbench just
170178
// as soon as we're initialized - ie: when we're trying to
171179
// run this runnable in the deferred case.
180+
} finally {
181+
// Signal that this operation has completed
182+
asyncPhaser.arriveAndDeregister();
172183
}
173184
}
174185
};
@@ -177,19 +188,27 @@ public void run() {
177188
}
178189

179190
private void setupAsyncDisplayThread(final boolean callDisplayAccess, final Display display) {
191+
// Register this thread with the phaser
192+
asyncPhaser.register();
180193
Thread asyncThread = new Thread() {
181194
@Override
182195
public void run() {
183196
if (callDisplayAccess)
184197
DisplayAccess.accessDisplayDuringStartup();
185-
display.asyncExec(() -> {
186-
synchronized (RCPTestWorkbenchAdvisor.class) {
187-
if (callDisplayAccess)
188-
asyncWithDisplayAccess = !isSTARTED() ? Boolean.TRUE : Boolean.FALSE;
189-
else
190-
asyncWithoutDisplayAccess = !isSTARTED() ? Boolean.TRUE : Boolean.FALSE;
191-
}
192-
});
198+
try {
199+
display.asyncExec(() -> {
200+
synchronized (RCPTestWorkbenchAdvisor.class) {
201+
if (callDisplayAccess)
202+
asyncWithDisplayAccess = !isSTARTED();
203+
else
204+
asyncWithoutDisplayAccess = !isSTARTED();
205+
}
206+
});
207+
} finally {
208+
// Signal that this operation has been scheduled (not necessarily executed yet)
209+
// This avoids deadlock since we're not waiting for execution on the UI thread
210+
asyncPhaser.arriveAndDeregister();
211+
}
193212
}
194213
};
195214
asyncThread.setDaemon(true);
@@ -199,6 +218,27 @@ public void run() {
199218
@Override
200219
public void postStartup() {
201220
super.postStartup();
221+
222+
// Wait for all async/sync operations to be scheduled (not blocking UI thread)
223+
try {
224+
// Wait up to 5 seconds for all operations to be scheduled
225+
// The main thread arrives and deregisters, waiting for all other registered threads
226+
asyncPhaser.awaitAdvanceInterruptibly(asyncPhaser.arrive(), 5, TimeUnit.SECONDS);
227+
} catch (TimeoutException e) {
228+
throw new AssertionError("Not all async/sync operations were scheduled within timeout", e);
229+
} catch (InterruptedException e) {
230+
Thread.currentThread().interrupt();
231+
throw new RuntimeException("Interrupted while waiting for async/sync operations", e);
232+
}
233+
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
237+
UITestUtil.processEventsUntil(() -> syncWithDisplayAccess != null && asyncWithDisplayAccess != null, 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();
241+
202242
synchronized (RCPTestWorkbenchAdvisor.class) {
203243
started = true;
204244
}

0 commit comments

Comments
 (0)