Skip to content

Commit 54b8aa2

Browse files
vogellaclaude
andcommitted
Fix race condition in CleanupAddon.subscribeRenderingChanged
The test ensureCleanUpAddonCleansUp was failing intermittently because CleanupAddon deferred container cleanup via Display.asyncExec(), creating a race condition where event loop pumping during part activation could interfere with async cleanup tasks. Root cause analysis: - When hidePart() is called on parts with REMOVE_ON_HIDE_TAG, it triggers synchronous EMF model change events via UIEventPublisher - Event delivery is fully synchronous: EventBroker.send() -> EventAdmin.sendEvent() -> Display.syncExec() -> handler execution - The subscribeRenderingChanged handler detected visCount==0 but deferred cleanup via asyncExec (line 352) - Meanwhile, activate() calls during hidePart() pumped the event loop (via focusGui/SWT focus handling), potentially consuming queued asyncExecs before they could observe the final state - This created timing-dependent behavior where cleanup could be skipped Fix: - Remove asyncExec wrapper from subscribeRenderingChanged's visCount==0 path - Since event delivery is synchronous, visCount already reflects the actual state at event time - no need to defer and re-check - Container is now hidden immediately when last renderable child becomes non-renderable, before any event loop pumping can interfere - Update test to use simple spinEventLoop + assertFalse instead of 30-second waitForCondition, as cleanup is now synchronous This fixes the flaky test that failed ~1-3% of the time even with the 30-second timeout. The subscribeTopicChildren handler still uses asyncExec for children removal/sash unwrapping, but toBeRendered cleanup is now deterministic. Fixes #3581 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1 parent 883c8c8 commit 54b8aa2

2 files changed

Lines changed: 10 additions & 17 deletions

File tree

bundles/org.eclipse.e4.ui.workbench.addons.swt/src/org/eclipse/e4/ui/workbench/addons/cleanupaddon/CleanupAddon.java

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -346,16 +346,13 @@ private void subscribeRenderingChanged(
346346
int visCount = modelService.countRenderableChildren(container);
347347

348348
// Remove stacks with no visible children from the display (but not the
349-
// model)
350-
final MElementContainer<MUIElement> theContainer = container;
351-
if (visCount == 0) {
352-
Display.getCurrent().asyncExec(() -> {
353-
int visCount1 = modelService.countRenderableChildren(theContainer);
354-
if (!isLastEditorStack(theContainer) && visCount1 == 0) {
355-
theContainer.setToBeRendered(false);
356-
transferPrimaryDataStackIfRemoved(theContainer);
357-
}
358-
});
349+
// model). Since event delivery is synchronous (EventBroker.send via
350+
// EventAdmin.sendEvent and Display.syncExec), the visCount reflects
351+
// the actual state at the time of this event and can be acted upon
352+
// immediately without deferring via asyncExec.
353+
if (visCount == 0 && !isLastEditorStack(container)) {
354+
container.setToBeRendered(false);
355+
transferPrimaryDataStackIfRemoved(container);
359356
} else if (container.getParent() != null) { // omit detached windows
360357
// if there are rendered elements but none are 'visible' we should
361358
// make the container invisible as well

tests/org.eclipse.e4.ui.tests/src/org/eclipse/e4/ui/tests/workbench/PartRenderingEngineTests.java

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,6 @@
6464
import org.eclipse.swt.widgets.Shell;
6565
import org.eclipse.swt.widgets.ToolBar;
6666
import org.eclipse.swt.widgets.Widget;
67-
import org.eclipse.ui.tests.harness.util.DisplayHelper;
6867
import org.junit.jupiter.api.AfterEach;
6968
import org.junit.jupiter.api.BeforeEach;
7069
import org.junit.jupiter.api.Disabled;
@@ -2447,12 +2446,9 @@ public void ensureCleanUpAddonCleansUp() {
24472446
assertTrue(partStackForPartBPartC.isToBeRendered(), " PartStack with children should be rendered");
24482447
partService.hidePart(partB);
24492448
partService.hidePart(partC);
2450-
// DisplayHelper.waitForCondition() handles event processing via Display.sleep()
2451-
// and retries. Calling spinEventLoop() here creates a race condition where
2452-
// events may be processed before CleanupAddon's asyncExec() is queued (line 352).
2453-
assertTrue(
2454-
DisplayHelper.waitForCondition(Display.getDefault(), 30_000,
2455-
() -> !partStackForPartBPartC.isToBeRendered()), "CleanupAddon should ensure that partStack is not rendered anymore, as all childs have been removed");
2449+
contextRule.spinEventLoop();
2450+
assertFalse(partStackForPartBPartC.isToBeRendered(),
2451+
"CleanupAddon should ensure that partStack is not rendered anymore, as all childs have been removed");
24562452
// PartStack with IPresentationEngine.NO_AUTO_COLLAPSE should not be removed
24572453
// even if children are removed
24582454
partService.hidePart(editor, true);

0 commit comments

Comments
 (0)