From e6e2ac98f986b393a6a76b9cf196fbf7fc816f7f Mon Sep 17 00:00:00 2001 From: Lars Vogel Date: Fri, 17 Apr 2026 05:29:05 +0200 Subject: [PATCH] Fix flaky ResourceInitialSelectionTest via pipeline scheduling rule FilteredItemsSelectionDialog's background pipeline has three jobs that read and write the same ContentProvider state: FilterHistoryJob mutates it via contentProvider.reset() and contentProvider.addHistoryItems(), FilterJob populates it via contentProvider.add() (inside fillContentProvider), and RefreshCacheJob reads it in contentProvider.reloadCache(). Without a shared scheduling rule they can run concurrently on different worker threads. Under the right timing, FilterHistoryJob.contentProvider.reset() clears the items set on one worker while FilterJob is iterating members() and adding items on another - wiping some or all of the populated items before the table is rendered. This manifested as the long-standing ResourceInitialSelectionTest flake (issue #294): sometimes the table was empty, sometimes a random subset of items survived in the wrong order. The race is reliably triggered by the ModifyListener on the pattern Text firing applyFilter() twice in quick succession during createDialogArea on slow runners, which schedules two FilterHistoryJobs that overlap with the FilterJob. Fix: share a per-dialog ISchedulingRule across filterHistoryJob, filterJob and refreshCacheJob. They now serialize; the race cannot occur. Separate dialogs still run in parallel. Also expose JOB_FAMILY (@noreference) and tag all four pipeline jobs with it, so tests can deterministically wait for the pipeline via Job.getJobManager().find(JOB_FAMILY). The item-count stability probe in ResourceInitialSelectionTest.waitForDialogRefresh() is replaced with that family-based wait - this also gets rid of the unreliable 5 s timeout that was the visible symptom. Fixes https://github.com/eclipse-platform/eclipse.platform.ui/issues/294 --- .../.settings/.api_filters | 8 +++ .../dialogs/FilteredItemsSelectionDialog.java | 58 +++++++++++++++++++ .../dialogs/ResourceInitialSelectionTest.java | 57 ++++++++---------- 3 files changed, 91 insertions(+), 32 deletions(-) diff --git a/bundles/org.eclipse.ui.workbench/.settings/.api_filters b/bundles/org.eclipse.ui.workbench/.settings/.api_filters index 4614ab0685b..d3d918f2683 100644 --- a/bundles/org.eclipse.ui.workbench/.settings/.api_filters +++ b/bundles/org.eclipse.ui.workbench/.settings/.api_filters @@ -1,5 +1,13 @@ + + + + + + + + diff --git a/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/dialogs/FilteredItemsSelectionDialog.java b/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/dialogs/FilteredItemsSelectionDialog.java index 5da18768f29..065b4cdc8e4 100644 --- a/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/dialogs/FilteredItemsSelectionDialog.java +++ b/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/dialogs/FilteredItemsSelectionDialog.java @@ -50,6 +50,7 @@ import org.eclipse.core.runtime.ProgressMonitorWrapper; import org.eclipse.core.runtime.Status; import org.eclipse.core.runtime.SubMonitor; +import org.eclipse.core.runtime.jobs.ISchedulingRule; import org.eclipse.core.runtime.jobs.Job; import org.eclipse.jface.action.Action; import org.eclipse.jface.action.ActionContributionItem; @@ -236,6 +237,17 @@ public abstract class FilteredItemsSelectionDialog extends SelectionStatusDialog */ private boolean isShownForTheFirstTime = true; + /** + * Job family under which all background filtering and refresh jobs of this + * dialog are grouped. Tests may use this with + * {@link org.eclipse.core.runtime.jobs.IJobManager#join(Object, org.eclipse.core.runtime.IProgressMonitor)} + * to deterministically wait for the filter/refresh pipeline to settle. + * + * @noreference This field is not intended to be referenced by clients. + * @since 3.138 + */ + public static final Object JOB_FAMILY = new Object(); + /** * Creates a new instance of the class. * @@ -250,10 +262,36 @@ public FilteredItemsSelectionDialog(Shell shell, boolean multi) { filterJob = new FilterJob(); contentProvider = new ContentProvider(); refreshCacheJob = new RefreshCacheJob(); + // All three jobs mutate / read the same ContentProvider state (items, + // duplicates, lastFilteredItems, ...). Without a common scheduling rule + // they can run concurrently on different worker threads, racing + // FilterHistoryJob.contentProvider.reset() against FilterJob's + // fillContentProvider() and silently emptying or corrupting the visible + // items. The rule is per-dialog, so separate dialogs still run in + // parallel. + ISchedulingRule pipelineRule = new ContentProviderSerialRule(); + filterHistoryJob.setRule(pipelineRule); + filterJob.setRule(pipelineRule); + refreshCacheJob.setRule(pipelineRule); itemsListSeparator = new ItemsListSeparator(WorkbenchMessages.FilteredItemsSelectionDialog_separatorLabel); selectionMode = NONE; } + /** + * Per-dialog mutex rule used to serialize the filter/refresh pipeline jobs. + */ + private static final class ContentProviderSerialRule implements ISchedulingRule { + @Override + public boolean contains(ISchedulingRule rule) { + return rule == this; + } + + @Override + public boolean isConflicting(ISchedulingRule rule) { + return rule == this; + } + } + /** * Creates a new instance of the class. Created dialog won't allow to select * more than one item. @@ -1315,6 +1353,11 @@ public IStatus runInUIThread(IProgressMonitor monitor) { return new Status(IStatus.OK, PlatformUI.PLUGIN_ID, IStatus.OK, EMPTY_STRING, null); } + @Override + public boolean belongsTo(Object family) { + return family == JOB_FAMILY; + } + } /** @@ -1420,6 +1463,11 @@ protected void canceling() { contentProvider.stopReloadingCache(); } + @Override + public boolean belongsTo(Object family) { + return family == JOB_FAMILY; + } + } private class RemoveHistoryItemAction extends Action { @@ -1854,6 +1902,11 @@ protected IStatus run(IProgressMonitor monitor) { return Status.OK_STATUS; } + @Override + public boolean belongsTo(Object family) { + return family == JOB_FAMILY; + } + } /** @@ -1977,6 +2030,11 @@ protected void filterContent(GranualProgressMonitor monitor) throws CoreExceptio } + @Override + public boolean belongsTo(Object family) { + return family == JOB_FAMILY; + } + } /** diff --git a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/dialogs/ResourceInitialSelectionTest.java b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/dialogs/ResourceInitialSelectionTest.java index a4c6fb2603a..0dc1be9f588 100644 --- a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/dialogs/ResourceInitialSelectionTest.java +++ b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/dialogs/ResourceInitialSelectionTest.java @@ -38,6 +38,7 @@ import org.eclipse.swt.widgets.Table; import org.eclipse.swt.widgets.TableItem; import org.eclipse.ui.PlatformUI; +import org.eclipse.ui.dialogs.FilteredItemsSelectionDialog; import org.eclipse.ui.dialogs.FilteredResourcesSelectionDialog; import org.eclipse.ui.internal.decorators.DecoratorManager; import org.eclipse.ui.tests.harness.util.DisplayHelper; @@ -125,8 +126,11 @@ public void testSingleSelectionAndOneInitialNonExistingSelectionWithInitialPatte dialog.open(); dialog.refresh(); - // Don't wait for full refresh - this test checks that invalid initial - // selections don't cause a selection before dialog is fully loaded + // Intentionally no waitForDialogRefresh: this asserts that during + // initial load, the dialog does not pre-select anything for an + // invalid initial element. After the refresh pipeline drains the + // dialog falls back to selecting row 0, which is a separate + // behavior not under test here. List selected = getSelectedItems(dialog); @@ -167,8 +171,10 @@ public void testSingleSelectionAndOneFilteredSelection() { dialog.open(); dialog.refresh(); - // Don't wait for full refresh - this test checks that filtered initial - // selections don't cause a selection before dialog is fully loaded + // Intentionally no waitForDialogRefresh: foofoo is filtered out by + // the *.txt pattern, so during initial load nothing is selected. + // After the refresh pipeline drains the dialog falls back to row 0, + // which is a separate behavior not under test here. List selected = getSelectedItems(dialog); @@ -274,8 +280,11 @@ public void testMultiSelectionAndTwoInitialNonExistingSelectionWithInitialPatter dialog.open(); dialog.refresh(); - // Don't wait for full refresh - this test checks that invalid initial - // selections don't cause a selection before dialog is fully loaded + // Intentionally no waitForDialogRefresh: this asserts that during + // initial load, the dialog does not pre-select anything for invalid + // initial elements. After the refresh pipeline drains the dialog + // falls back to selecting row 0, which is a separate behavior not + // under test here. List selected = getSelectedItems(dialog); @@ -448,37 +457,21 @@ private void processUIEvents() { } /** - * Wait for dialog refresh jobs to complete and process UI events. - * This ensures background jobs finish before assertions are made. + * Wait for the dialog's background filter/refresh jobs to complete. + *

+ * The dialog schedules a chain of jobs on open/refresh: + * {@code FilterHistoryJob → FilterJob → RefreshCacheJob → RefreshJob}. All + * four are tagged with {@link FilteredItemsSelectionDialog#JOB_FAMILY}, so + * we wait for that family to drain. The 30 s ceiling is a deadlock guard; + * the pipeline usually completes within a few hundred milliseconds. */ private void waitForDialogRefresh() { Display display = PlatformUI.getWorkbench().getDisplay(); - - // The dialog performs async operations (FilterHistoryJob → FilterJob → - // RefreshCacheJob → RefreshJob) to filter and populate the table after refresh() - // We need to wait for the table item count to stabilize before checking - // selection state. The count can temporarily be non-zero after the history - // refresh, then change again when FilterJob populates actual results. - // Selection is applied only in the final refresh, so we must wait for - // stability rather than just for any non-zero count. - int[] lastCount = { -1 }; - DisplayHelper.waitForCondition(display, 5000, () -> { + DisplayHelper.waitForCondition(display, 30_000L, () -> { processUIEvents(); - try { - Table table = (Table) ((Composite) ((Composite) ((Composite) dialog.getShell().getChildren()[0]) - .getChildren()[0]).getChildren()[0]).getChildren()[3]; - int count = table.getItemCount(); - if (count > 0 && count == lastCount[0]) { - return true; // stable non-zero count: all refreshes have completed - } - lastCount[0] = count; - return false; - } catch (Exception e) { - return false; - } + return Job.getJobManager().find(FilteredItemsSelectionDialog.JOB_FAMILY).length == 0; }); - - // Final event loop processing to pick up any trailing async tasks + // Final event loop processing to pick up any trailing asyncExecs. processUIEvents(); }