From 7b5e27f4e9acd46cea534bc97674d5013e82bd9d Mon Sep 17 00:00:00 2001 From: Lars Vogel Date: Tue, 4 Nov 2025 22:32:40 +0100 Subject: [PATCH 1/4] Fix race condition in RCPTestWorkbenchAdvisor test harness Introduce a Phaser to ensure async/sync operations are scheduled before postStartup marks the workbench as started. This prevents timing issues where test variables (asyncWithDisplayAccess, syncWithDisplayAccess, etc.) might not be set correctly due to the workbench being marked as started before async runnables execute. Changes: - Add Phaser to coordinate async/sync thread operations - Register/deregister threads when scheduling display operations - Wait for all operations to be scheduled in postStartup (with 5s timeout) - Process event loop to ensure async runnables execute before marking started - Simplify Boolean assignments (remove unnecessary ternary operators) This ensures deterministic behavior in test harness initialization and prevents flaky test failures due to race conditions. --- .../harness/util/RCPTestWorkbenchAdvisor.java | 74 ++++++++++++++----- 1 file changed, 57 insertions(+), 17 deletions(-) diff --git a/tests/org.eclipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util/RCPTestWorkbenchAdvisor.java b/tests/org.eclipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util/RCPTestWorkbenchAdvisor.java index 670ed8276af..539808432a9 100644 --- a/tests/org.eclipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util/RCPTestWorkbenchAdvisor.java +++ b/tests/org.eclipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util/RCPTestWorkbenchAdvisor.java @@ -13,6 +13,10 @@ *******************************************************************************/ package org.eclipse.ui.tests.harness.util; +import java.util.concurrent.Phaser; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; + import org.eclipse.jface.preference.IPreferenceStore; import org.eclipse.swt.SWTException; import org.eclipse.swt.widgets.Display; @@ -44,6 +48,10 @@ public class RCPTestWorkbenchAdvisor extends WorkbenchAdvisor { private static boolean started = false; + // Use a Phaser to ensure async operations are scheduled before postStartup + // Each thread registers itself and arrives when the operation is scheduled + private static final Phaser asyncPhaser = new Phaser(1); // 1 for the main thread + public static boolean isSTARTED() { synchronized (RCPTestWorkbenchAdvisor.class) { return started; @@ -122,12 +130,10 @@ public void eventLoopIdle(final Display display) { public void preStartup() { super.preStartup(); final Display display = Display.getCurrent(); + if (display != null) { display.asyncExec(() -> { - if (isSTARTED()) - asyncDuringStartup = Boolean.FALSE; - else - asyncDuringStartup = Boolean.TRUE; + asyncDuringStartup = !isSTARTED(); }); } @@ -151,6 +157,8 @@ public void preStartup() { } private void setupSyncDisplayThread(final boolean callDisplayAccess, final Display display) { + // Register this thread with the phaser + asyncPhaser.register(); Thread syncThread = new Thread() { @Override public void run() { @@ -158,17 +166,19 @@ public void run() { DisplayAccess.accessDisplayDuringStartup(); try { display.syncExec(() -> { - synchronized (RCPTestWorkbenchAdvisor.class) { - if (callDisplayAccess) - syncWithDisplayAccess = !isSTARTED() ? Boolean.TRUE : Boolean.FALSE; - else - syncWithoutDisplayAccess = !isSTARTED() ? Boolean.TRUE : Boolean.FALSE; + if (callDisplayAccess) { + syncWithDisplayAccess = !isSTARTED(); + } else { + syncWithoutDisplayAccess = !isSTARTED(); } }); } catch (SWTException e) { // this can happen because we shut down the workbench just // as soon as we're initialized - ie: when we're trying to // run this runnable in the deferred case. + } finally { + // Signal that this operation has completed + asyncPhaser.arriveAndDeregister(); } } }; @@ -177,19 +187,26 @@ public void run() { } private void setupAsyncDisplayThread(final boolean callDisplayAccess, final Display display) { + // Register this thread with the phaser + asyncPhaser.register(); Thread asyncThread = new Thread() { @Override public void run() { if (callDisplayAccess) DisplayAccess.accessDisplayDuringStartup(); - display.asyncExec(() -> { - synchronized (RCPTestWorkbenchAdvisor.class) { - if (callDisplayAccess) - asyncWithDisplayAccess = !isSTARTED() ? Boolean.TRUE : Boolean.FALSE; - else - asyncWithoutDisplayAccess = !isSTARTED() ? Boolean.TRUE : Boolean.FALSE; - } - }); + try { + display.asyncExec(() -> { + if (callDisplayAccess) { + asyncWithDisplayAccess = !isSTARTED(); + } else { + asyncWithoutDisplayAccess = !isSTARTED(); + } + }); + } finally { + // Signal that this operation has been scheduled (not necessarily executed yet) + // This avoids deadlock since we're not waiting for execution on the UI thread + asyncPhaser.arriveAndDeregister(); + } } }; asyncThread.setDaemon(true); @@ -199,6 +216,29 @@ public void run() { @Override public void postStartup() { super.postStartup(); + + // Wait for all async/sync operations to be scheduled (not blocking UI thread) + try { + // Wait up to 5 seconds for all operations to be scheduled + // The main thread arrives and deregisters, waiting for all other registered threads + asyncPhaser.awaitAdvanceInterruptibly(asyncPhaser.arrive(), 5, TimeUnit.SECONDS); + } catch (TimeoutException e) { + // Log warning but don't throw - we need to mark as started to avoid breaking subsequent tests + System.err.println("WARNING: Not all async/sync operations were scheduled within timeout"); + e.printStackTrace(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + System.err.println("WARNING: Interrupted while waiting for async/sync operations"); + } + + // Pump the event loop to ensure async runnables execute before marking as started + // This prevents the original race condition where async variables might not be set yet + // Wait until the variables that should be set during startup are actually set to TRUE + UITestUtil.processEventsUntil(() -> Boolean.TRUE.equals(syncWithDisplayAccess) && Boolean.TRUE.equals(asyncWithDisplayAccess), 5000); + // Process any remaining events to allow variables that should NOT be set during startup + // to accidentally execute (to detect regression) + UITestUtil.processEvents(); + synchronized (RCPTestWorkbenchAdvisor.class) { started = true; } From 48399e0f525a6d1f067cfec96f1c676fbff0a288 Mon Sep 17 00:00:00 2001 From: Eclipse Platform Bot Date: Tue, 4 Nov 2025 21:42:08 +0000 Subject: [PATCH 2/4] Version bump(s) for 4.38 stream --- tests/org.eclipse.ui.tests.harness/META-INF/MANIFEST.MF | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/org.eclipse.ui.tests.harness/META-INF/MANIFEST.MF b/tests/org.eclipse.ui.tests.harness/META-INF/MANIFEST.MF index 259fee3e993..8cdb2f2c5fb 100644 --- a/tests/org.eclipse.ui.tests.harness/META-INF/MANIFEST.MF +++ b/tests/org.eclipse.ui.tests.harness/META-INF/MANIFEST.MF @@ -2,7 +2,7 @@ Manifest-Version: 1.0 Bundle-ManifestVersion: 2 Bundle-Name: Harness Plug-in Bundle-SymbolicName: org.eclipse.ui.tests.harness;singleton:=true -Bundle-Version: 1.10.600.qualifier +Bundle-Version: 1.10.700.qualifier Eclipse-BundleShape: dir Require-Bundle: org.eclipse.ui, org.eclipse.core.runtime, From 1b8a9f30ff40866457a298b9dd03bf5752678941 Mon Sep 17 00:00:00 2001 From: Lars Vogel Date: Fri, 7 Nov 2025 12:43:51 +0100 Subject: [PATCH 3/4] Replace Phaser with CountDownLatch for reliable synchronization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../harness/util/RCPTestWorkbenchAdvisor.java | 91 ++++++++++--------- 1 file changed, 48 insertions(+), 43 deletions(-) diff --git a/tests/org.eclipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util/RCPTestWorkbenchAdvisor.java b/tests/org.eclipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util/RCPTestWorkbenchAdvisor.java index 539808432a9..87cbdc1eaa0 100644 --- a/tests/org.eclipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util/RCPTestWorkbenchAdvisor.java +++ b/tests/org.eclipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util/RCPTestWorkbenchAdvisor.java @@ -13,9 +13,8 @@ *******************************************************************************/ package org.eclipse.ui.tests.harness.util; -import java.util.concurrent.Phaser; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; import org.eclipse.jface.preference.IPreferenceStore; import org.eclipse.swt.SWTException; @@ -48,9 +47,9 @@ public class RCPTestWorkbenchAdvisor extends WorkbenchAdvisor { private static boolean started = false; - // Use a Phaser to ensure async operations are scheduled before postStartup - // Each thread registers itself and arrives when the operation is scheduled - private static final Phaser asyncPhaser = new Phaser(1); // 1 for the main thread + // CountDownLatch to wait for async/sync operations with DisplayAccess to complete + // We need to wait for 2 operations: asyncWithDisplayAccess and syncWithDisplayAccess + private static CountDownLatch displayAccessLatch = null; public static boolean isSTARTED() { synchronized (RCPTestWorkbenchAdvisor.class) { @@ -131,6 +130,9 @@ public void preStartup() { super.preStartup(); final Display display = Display.getCurrent(); + // Initialize the latch to wait for 2 operations with DisplayAccess + displayAccessLatch = new CountDownLatch(2); + if (display != null) { display.asyncExec(() -> { asyncDuringStartup = !isSTARTED(); @@ -157,8 +159,6 @@ public void preStartup() { } private void setupSyncDisplayThread(final boolean callDisplayAccess, final Display display) { - // Register this thread with the phaser - asyncPhaser.register(); Thread syncThread = new Thread() { @Override public void run() { @@ -168,6 +168,10 @@ public void run() { display.syncExec(() -> { if (callDisplayAccess) { syncWithDisplayAccess = !isSTARTED(); + // Count down after the runnable executes + if (displayAccessLatch != null) { + displayAccessLatch.countDown(); + } } else { syncWithoutDisplayAccess = !isSTARTED(); } @@ -176,9 +180,9 @@ public void run() { // this can happen because we shut down the workbench just // as soon as we're initialized - ie: when we're trying to // run this runnable in the deferred case. - } finally { - // Signal that this operation has completed - asyncPhaser.arriveAndDeregister(); + if (callDisplayAccess && displayAccessLatch != null) { + displayAccessLatch.countDown(); + } } } }; @@ -187,26 +191,22 @@ public void run() { } private void setupAsyncDisplayThread(final boolean callDisplayAccess, final Display display) { - // Register this thread with the phaser - asyncPhaser.register(); Thread asyncThread = new Thread() { @Override public void run() { if (callDisplayAccess) DisplayAccess.accessDisplayDuringStartup(); - try { - display.asyncExec(() -> { - if (callDisplayAccess) { - asyncWithDisplayAccess = !isSTARTED(); - } else { - asyncWithoutDisplayAccess = !isSTARTED(); + display.asyncExec(() -> { + if (callDisplayAccess) { + asyncWithDisplayAccess = !isSTARTED(); + // Count down after the runnable executes + if (displayAccessLatch != null) { + displayAccessLatch.countDown(); } - }); - } finally { - // Signal that this operation has been scheduled (not necessarily executed yet) - // This avoids deadlock since we're not waiting for execution on the UI thread - asyncPhaser.arriveAndDeregister(); - } + } else { + asyncWithoutDisplayAccess = !isSTARTED(); + } + }); } }; asyncThread.setDaemon(true); @@ -217,28 +217,33 @@ public void run() { public void postStartup() { super.postStartup(); - // Wait for all async/sync operations to be scheduled (not blocking UI thread) - try { - // Wait up to 5 seconds for all operations to be scheduled - // The main thread arrives and deregisters, waiting for all other registered threads - asyncPhaser.awaitAdvanceInterruptibly(asyncPhaser.arrive(), 5, TimeUnit.SECONDS); - } catch (TimeoutException e) { - // Log warning but don't throw - we need to mark as started to avoid breaking subsequent tests - System.err.println("WARNING: Not all async/sync operations were scheduled within timeout"); - e.printStackTrace(); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - System.err.println("WARNING: Interrupted while waiting for async/sync operations"); + // Wait for async/sync operations with DisplayAccess to complete execution + if (displayAccessLatch != null) { + try { + // Wait up to 5 seconds for operations with DisplayAccess to complete + // This ensures they execute BEFORE we mark started = true + boolean completed = displayAccessLatch.await(5, TimeUnit.SECONDS); + if (!completed) { + System.err.println("WARNING: Timeout waiting for async/sync operations with DisplayAccess"); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + System.err.println("WARNING: Interrupted while waiting for async/sync operations"); + } } - // Pump the event loop to ensure async runnables execute before marking as started - // This prevents the original race condition where async variables might not be set yet - // Wait until the variables that should be set during startup are actually set to TRUE - UITestUtil.processEventsUntil(() -> Boolean.TRUE.equals(syncWithDisplayAccess) && Boolean.TRUE.equals(asyncWithDisplayAccess), 5000); - // Process any remaining events to allow variables that should NOT be set during startup - // to accidentally execute (to detect regression) - UITestUtil.processEvents(); + // Process remaining events to allow any pending runnables to execute + // This gives operations WITHOUT DisplayAccess a chance to run if there are bugs + Display display = Display.getCurrent(); + if (display != null) { + // Process all pending events + while (display.readAndDispatch()) { + // Keep processing + } + } + // Now mark as started - operations with DisplayAccess should have completed + // Operations without DisplayAccess should still be pending (deferred) synchronized (RCPTestWorkbenchAdvisor.class) { started = true; } From 0a190b3a3401408db8c13b8f270842b5c5a73eda Mon Sep 17 00:00:00 2001 From: Lars Vogel Date: Wed, 12 Nov 2025 18:25:43 +0100 Subject: [PATCH 4/4] Fix race condition in RCPTestWorkbenchAdvisor by removing premature event processing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous fix added a CountDownLatch to wait for operations with DisplayAccess, but also included a display.readAndDispatch() loop that processed ALL pending events in postStartup(). This caused deferred operations (without DisplayAccess) to execute before startup completed, violating the contract that such operations should be deferred until after startup. This change removes the display.readAndDispatch() loop while keeping the latch-based synchronization for operations with DisplayAccess. The test expects: - Operations WITH DisplayAccess → run during startup - Operations WITHOUT DisplayAccess → deferred until after startup - Direct asyncExec from UI thread → deferred until after startup 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../ui/tests/harness/util/RCPTestWorkbenchAdvisor.java | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/tests/org.eclipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util/RCPTestWorkbenchAdvisor.java b/tests/org.eclipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util/RCPTestWorkbenchAdvisor.java index 87cbdc1eaa0..5f5f19feeca 100644 --- a/tests/org.eclipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util/RCPTestWorkbenchAdvisor.java +++ b/tests/org.eclipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util/RCPTestWorkbenchAdvisor.java @@ -232,16 +232,6 @@ public void postStartup() { } } - // Process remaining events to allow any pending runnables to execute - // This gives operations WITHOUT DisplayAccess a chance to run if there are bugs - Display display = Display.getCurrent(); - if (display != null) { - // Process all pending events - while (display.readAndDispatch()) { - // Keep processing - } - } - // Now mark as started - operations with DisplayAccess should have completed // Operations without DisplayAccess should still be pending (deferred) synchronized (RCPTestWorkbenchAdvisor.class) {