Skip to content

CountDownLatch Synchronization for RCPTestWorkbenchAdvisor #3429

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+60 −15
Open

CountDownLatch Synchronization for RCPTestWorkbenchAdvisor #3429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tests/org.eclipse.ui.tests.harness/META-INF/MANIFEST.MF
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
73 changes: 59 additions & 14 deletions ...lipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util/RCPTestWorkbenchAdvisor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@
*******************************************************************************/
package org.eclipse.ui.tests.harness.util;

import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;

import org.eclipse.jface.preference.IPreferenceStore;
import org.eclipse.swt.SWTException;
import org.eclipse.swt.widgets.Display;
Expand Down Expand Up @@ -44,6 +47,9 @@ public class RCPTestWorkbenchAdvisor extends WorkbenchAdvisor {

private static boolean started = false;

// Use a CountDownLatch to ensure async operations complete before postStartup
private static final CountDownLatch asyncLatch = new CountDownLatch(4);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: wouldn't it be cleaner to use something like a Phaser and make every thread register itself and then deregister/arrive when being executed (like now done with asyncLatch.countDown()), so that the latch's count does not need to magically match the number of thread created?


public static boolean isSTARTED() {
synchronized (RCPTestWorkbenchAdvisor.class) {
return started;
Expand Down Expand Up @@ -122,12 +128,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();
});
}

Expand Down Expand Up @@ -160,15 +164,18 @@ public void run() {
display.syncExec(() -> {
synchronized (RCPTestWorkbenchAdvisor.class) {
if (callDisplayAccess)
syncWithDisplayAccess = !isSTARTED() ? Boolean.TRUE : Boolean.FALSE;
syncWithDisplayAccess = !isSTARTED();
else
syncWithoutDisplayAccess = !isSTARTED() ? Boolean.TRUE : Boolean.FALSE;
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
asyncLatch.countDown();
}
}
};
Expand All @@ -182,14 +189,20 @@ private void setupAsyncDisplayThread(final boolean callDisplayAccess, final Disp
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(() -> {
synchronized (RCPTestWorkbenchAdvisor.class) {
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
asyncLatch.countDown();
}
}
};
asyncThread.setDaemon(true);
Expand All @@ -199,6 +212,38 @@ 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
boolean completed = asyncLatch.await(5, TimeUnit.SECONDS);
if (!completed) {
throw new AssertionError("Not all async/sync operations were scheduled within timeout");
}
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new RuntimeException("Interrupted while waiting for async/sync operations", e);
}

// 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
Display display = Display.getCurrent();
if (display != null) {
// Process pending async events multiple times to ensure they execute
for (int i = 0; i < 10; i++) {
while (display.readAndDispatch()) {
// Keep processing events
}
// Small yield to allow any final async operations to complete
try {
Thread.sleep(10);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
break;
}
}
}
Comment on lines +230 to +245
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this quite complex event processing really necessary? All relevant runnables are guaranteed to be scheduled here, so display.readAndDispatch() should not yield false until they all have been processed. You just need to make sure that those runnables that are not supposed to be executed during startup had the chance to accidentally be executed (to detect regression). So wouldn't be something like this be sufficient?

Suggested change
Display display = Display.getCurrent();
if (display != null) {
// Process pending async events multiple times to ensure they execute
for (int i = 0; i < 10; i++) {
while (display.readAndDispatch()) {
// Keep processing events
}
// Small yield to allow any final async operations to complete
try {
Thread.sleep(10);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
break;
}
}
}
UITestUtil.processEventsUntil(() -> syncWithDisplayAccess && asyncWithDisplayAccess, 5000);
UITestUtil.processEvents();


synchronized (RCPTestWorkbenchAdvisor.class) {
started = true;
}
Expand Down
Loading