Never resolve the target platform from PDELabelProvider - #2417
Open
vogella wants to merge 1 commit into
Open
Conversation
vogella
force-pushed
the
label-provider-no-target-resolution
branch
from
August 11, 2026 08:49
3679685 to
d2740ce
Compare
vogella
marked this pull request as ready for review
August 11, 2026 09:20
There was a problem hiding this comment.
Pull request overview
This PR prevents PDELabelProvider (which runs on the UI thread) from triggering target platform resolution via plug-in and feature model lookups, by introducing “models available?” guards and background initialization paths that refresh viewer labels once initialization completes.
Changes:
- Add guarded accessors in
PDELabelProviderthat avoid target resolution on the UI thread and trigger async label refresh once models are ready. - Add
initializeInBackground(...)entry points toPluginModelManagerandFeatureModelManager, backed by a sharedBackgroundInitializationhelper. - Add
PDELabelProviderTestand wire it intoAllPDETeststo verify the plug-in import decoration behavior before/after models are available.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/PDELabelProvider.java | Guards label/image computation to avoid model queries that can resolve the target platform on the UI thread; refreshes labels after background init. |
| ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/ui/tests/util/PDELabelProviderTest.java | Adds regression tests for the plug-in import decoration behavior when models are/aren’t available. |
| ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/ui/tests/AllPDETests.java | Includes the new label provider test in the main test suite. |
| ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/PluginModelManager.java | Adds background initialization API for plug-in model table population. |
| ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/FeatureModelManager.java | Adds background initialization API for feature model loading. |
| ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/BackgroundInitialization.java | Introduces shared helper to schedule init jobs and invoke callbacks once initialization completes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
vogella
force-pushed
the
label-provider-no-target-resolution
branch
from
August 11, 2026 10:07
d2740ce to
eda34bb
Compare
Member
|
Such a large change that touches a lot of code in central parts of the model manager and introduces more concurrency should be reviewed carefully. |
vogella
force-pushed
the
label-provider-no-target-resolution
branch
from
August 12, 2026 15:35
eda34bb to
2fe82f1
Compare
Contributor
Author
Why I think it is worth doing: with an m2e Maven location in the target, sampling the main thread every 500 ms during startup put roughly 8 seconds inside target resolution reached from getObjectText(ISiteFeature), with a Category Definition editor restored from the previous session. On Windows with Defender active it gets considerably worse. On risk: if I got a guard wrong, the visible effect is a label that stays plain until the refresh event arrives, not a wrong model state. PDELabelProviderTest covers both paths, plug-ins and features. I am running it in my own patched IDE build now and will report back after more production use. |
vogella
force-pushed
the
label-provider-no-target-resolution
branch
2 times, most recently
from
August 13, 2026 08:15
e1277aa to
dcc8e23
Compare
Label providers run on the UI thread, so they must not ask a question whose
answer requires resolving the target platform. PDELabelProvider did, through
both model managers, and either one can freeze the IDE for minutes when the
target contains an m2e Maven location.
Sampling the main thread every 500 ms while an IDE with a Category Definition
editor started up put 10 consecutive samples, roughly 8 seconds, in this chain:
CategorySection.createClient()
TreeViewer.setInput()
CategoryLabelProvider.getText()
PDELabelProvider.getObjectText(ISiteFeature)
FeatureModelManager.findFeatureModel()
FeatureModelManager.init() locked
ExternalFeatureModelManager.initialize() locked
TargetPlatformHelper.getWorkspaceTargetResolved()
MavenTargetLocation.resolveArtifacts() locked
The reported manifest editor freeze is the same shape through the other
manager:
RequiresSection.initialize()
TableViewer.setInput()
PDELabelProvider.getObjectImage(ImportObject)
ImportObject.isResolved()
PluginRegistry.findModel()
PluginModelManager.findEntry() locked
getEntryTable() -> initializeTable()
TargetPlatformHelper.getWorkspaceTargetResolved()
PluginModelManager and FeatureModelManager are two doors into the same room.
Whichever is opened first pays for the full resolution and caches the resolved
target, which is why the sampled run never entered initializeTable at all: the
feature manager had already paid, and the plug-in manager got in free. Fixing
one door alone would not change what the user sees.
Both are guarded now. arePluginModelsAvailable() and areFeatureModelsAvailable()
build on the isInitialized() fast paths both managers already had. While the
models are unknown the plain label is returned and initialization is scheduled
through the new initializeInBackground(Runnable) on each manager, sharing the
race handling in BackgroundInitialization. When it completes the label provider
fires a LabelProviderChangedEvent, so every viewer sharing it repaints with the
resolved state. Unresolved imports and missing features still get their error
overlay once the models are there.
Guarded call sites, none of which had a non-resolving path before:
plug-in models getObjectImage(ImportObject), getObjectImage(PackageObject),
getObjectImage(IProductPlugin), getObjectImage(IFeatureImport),
getObjectImage(IFeaturePlugin), getObjectText(IPluginBase),
getObjectText(ImportObject), getObjectText(IPluginImport),
getObjectText(BundleDescription), getObjectText(FeaturePlugin),
getObjectText(ISiteBundle)
feature models getObjectText(ISiteFeature), getObjectText(FeatureImport),
getObjectImage(IProductFeature), getObjectImage(IFeatureChild),
getObjectImage(IFeatureImport)
getSystemBundleInfo() also stopped assuming that system.bundle resolves; it
dereferenced a possibly null model.
initializeInBackground() is new on both managers, but both live in
org.eclipse.pde.internal.core, so this is not published API. Firing a
PluginModelDelta after initialization was rejected on purpose: it would make
PDERegistryStrategy create the extension registry, FeatureRebuilder touch all
feature projects and PluginsView add every entry one by one.
Both isInitialized() implementations had to become honest and non-blocking for
the guards to work at all. FeatureModelManager reported success as soon as
fActiveModels was assigned, which happens long before the external features are
read, so a caller could be told the models are there and then block on the
init() monitor. It now reports the completion of init(). PluginModelManager
answered while holding fEntriesSynchronizer, which the background job holds for
the whole resolution, so asking whether initialization is needed would wait for
exactly that initialization. fEntries is volatile now and is read without the
lock; it is only ever assigned a fully populated table.
Scheduling that initialization must not take the resolution lock either.
initializeInBackground() created its job while holding the very monitor that
init() and initializeTable() hold across the resolution, so a label provider
painting during a target reload would block on it and the guard would have
bought nothing. Each manager now has a private fInitializationJobLock that is
only ever held for the lazy Job.create.
ModelManagerBackgroundInitializationTest holds the resolution lock on another
thread and asserts that initializeInBackground() returns before that thread
releases it. Both cases fail without the dedicated lock.
getObjectImage(IFeaturePlugin) needed the guard in two places. isFragment()
reaches the plug-in registry through FeaturePlugin.getPluginBase() exactly like
the error overlay does, so one hoisted check now covers both and the plain
plug-in image is returned while the models are unknown.
A failed external read no longer wedges the feature manager. init() fast-returns
once fActiveModels is set, so leaving fModelsAvailable false on the way out
would have kept isInitialized() false for good. It is set in a finally now,
which is the state the code reported before this change.
The lock assertion moved into ConcurrencyUtil so that PDELabelProviderTest can
hold fEntriesSynchronizer while asking for a feature plug-in image.
Two schema paths resolve underneath as well. getObjectImage(IPluginElement)
looks the extension point up in the schema registry, which builds the extension
registry from all workspace and external models, and reaches getState() even
when that registry is cached. The ISchemaInclude branch of getImage() resolves a
schema:// include through PluginRegistry.findEntry. Both are guarded now and
fall back to the undecorated image, so the Extensions page of the manifest
editor and the schema editor stop resolving too.
vogella
force-pushed
the
label-provider-no-target-resolution
branch
from
August 13, 2026 11:41
dcc8e23 to
87fcbe2
Compare
Contributor
Author
|
Found a few more places which would trigger an eager target platform load. As I'm only working for the client who had this issue in 2 weeks again, I post-phone this fix to 4.42 as I cannot verified the impact on startup time shortly before our release. Thanks @HannesWell for the feedback. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Label providers run on the UI thread, so they must not ask a question whose answer requires resolving the target platform.
PDELabelProviderdid, through bothPluginModelManagerandFeatureModelManager, and either one can freeze the IDE for a long time when the target contains an m2e Maven location, because resolution runs a fullMavenExecutionContextand bnd-wraps every transitive artifact while holding the manager's monitor.Sampling the main thread every 500 ms during IDE startup, with a Category Definition editor restored from the previous session, put 10 consecutive samples, roughly 8 seconds, inside target resolution reached from
getObjectText(ISiteFeature).The originally reported manifest editor freeze is the same shape through the other manager, from
getObjectImage(ImportObject).Both are guarded now, building on the
isInitialized()fast paths the two managers already had.While the models are unknown the plain label is returned and initialization is scheduled in a background job; when it finishes the label provider fires a
LabelProviderChangedEvent, so every viewer sharing it repaints with the resolved state.Unresolved imports and missing features still get their error overlay once the models are available.
The two managers are two doors into the same room: whichever is opened first pays for the full resolution and caches the resolved target.
That is visible in the sampled run, which never entered
initializeTableat all because the feature manager had already paid.Guarding only one door would not have changed what the user sees, which is why both are in this change.
And the originally reported manifest editor freeze, through the other manager:
Both
isInitialized()implementations had to become honest and non-blocking, otherwise the guards would have replaced one freeze with another.FeatureModelManagerreported success as soon asfActiveModelswas assigned, which happens long before the external features are read, so a caller could be told the models are there and then block on theinit()monitor.It now reports the completion of
init().PluginModelManageranswered while holdingfEntriesSynchronizer, which the background job holds for the whole resolution, so asking whether initialization is needed would have waited for exactly that initialization;fEntriesis volatile now and read without the lock, and it is only ever assigned a fully populated table.PDELabelProviderTestcovers both paths.For plug-ins, an unresolvable import is still decorated once the models are available and gets the plain image while they are not.
For features, the same holds for a product feature, a site feature falls back to its URL while the models are unavailable, and resolves to the label of a workspace feature once they are there.
Manual check: fresh workspace, a target with an m2e Maven location using
dependencyDepth="infinite"and a cold~/.m2, with a category definition editor and a manifest editor open from the previous session.Both should render immediately with plain labels and fill in once resolution finishes in the background.