From e306ca5022a4a6e73879ebcf7590eba87352c41b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 5 Nov 2025 06:47:05 +0000 Subject: [PATCH 1/4] Initial plan From 3b0a80b661529762a08e1ce990cb9aa35ea0829f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 5 Nov 2025 06:55:34 +0000 Subject: [PATCH 2/4] Fix race condition in target platform loading causing IllegalStateException - Add profile-level synchronization to prevent concurrent access to the same P2 profile - Refresh profile from registry after engine.perform() operations to maintain timestamp consistency - Prevents "Profile is not current" errors when multiple ITargetDefinition instances share the same profile Co-authored-by: laeubi <1331477+laeubi@users.noreply.github.com> --- .../internal/core/target/P2TargetUtils.java | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/P2TargetUtils.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/P2TargetUtils.java index 39fd2d1ef44..b8e9036c8c4 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/P2TargetUtils.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/P2TargetUtils.java @@ -195,6 +195,13 @@ public class P2TargetUtils { */ private static final Map SYNCHRONIZERS = new WeakHashMap<>(); + /** + * Locks for profile operations, keyed by profile ID. This ensures that operations + * on the same profile (which can be shared by multiple ITargetDefinition instances) + * are properly synchronized to prevent timestamp conflicts. + */ + private static final ConcurrentHashMap PROFILE_LOCKS = new ConcurrentHashMap<>(); + /** * Table mapping of ITargetDefinition and IFileArtifactRepository */ @@ -799,6 +806,18 @@ public static IQueryResult getIUs(ITargetDefinition target, IP return synchronizer.getProfile().query(QueryUtil.createIUAnyQuery(), null); } + /** + * Gets or creates a lock object for the given profile ID. This ensures that + * operations on the same profile (which can be shared by multiple ITargetDefinition + * instances) are properly synchronized. + * + * @param profileId the profile identifier + * @return a lock object for the profile + */ + private static Object getProfileLock(String profileId) { + return PROFILE_LOCKS.computeIfAbsent(profileId, k -> new Object()); + } + /** * Synchronize the profile and the target definition managed by this synchronizer. On return the profile will * be resolved and correctly match the given target. The IUBundleContainers associated with @@ -814,6 +833,19 @@ public static IQueryResult getIUs(ITargetDefinition target, IP * @throws CoreException if there was a problem synchronizing */ public synchronized void synchronize(ITargetDefinition target, IProgressMonitor monitor) throws CoreException { + // Additional synchronization on the profile ID to prevent race conditions when + // multiple ITargetDefinition instances (with different P2TargetUtils instances) + // share the same profile ID based on the target file path + String profileId = getProfileId(target); + synchronized (getProfileLock(profileId)) { + synchronizeInternal(target, monitor); + } + } + + /** + * Internal implementation of synchronize that runs within the profile lock. + */ + private void synchronizeInternal(ITargetDefinition target, IProgressMonitor monitor) throws CoreException { SubMonitor progress = SubMonitor.convert(monitor, 100); IProfile profile = getProfile(); // Happiness if we have a profile and it checks out or if we can load one and it checks out. @@ -845,6 +877,12 @@ public synchronized void synchronize(ITargetDefinition target, IProgressMonitor } else { resolveWithSlicer(target, profile, progress.split(60)); } + // Refresh the profile from registry as resolve operations may have updated it + profile = getProfileRegistry().getProfile(profile.getProfileId()); + if (profile == null) { + throw new CoreException( + Status.error("Profile was removed after resolving: " + getProfileId(target))); //$NON-NLS-1$ + } fProfile = profile; // If we are updating a profile then delete the old snapshot on success. notify(target, progress.split(15)); @@ -1184,6 +1222,14 @@ public IQueryable getMetadata(IProgressMonitor monitor) { throw new CoreException(result); } + // Refresh the profile from the registry after perform, as the perform operation + // creates a new profile snapshot with an updated timestamp + profile = getProfileRegistry().getProfile(profile.getProfileId()); + if (profile == null) { + throw new CoreException( + Status.error("Profile was removed after provisioning: " + getProfileId(target))); //$NON-NLS-1$ + } + // Now that we have a plan with all the binary and explicit bundles, do a second pass and add // in all the source. try { From 06efad84695c5f2f8f9effd6288b96336cee8a29 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 5 Nov 2025 06:58:55 +0000 Subject: [PATCH 3/4] Address code review feedback: extract duplicate profile refresh logic - Extract refreshProfile() helper method to reduce code duplication - Add documentation about PROFILE_LOCKS memory considerations - Improve code maintainability Co-authored-by: laeubi <1331477+laeubi@users.noreply.github.com> --- .../internal/core/target/P2TargetUtils.java | 34 +++++++++++++------ 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/P2TargetUtils.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/P2TargetUtils.java index b8e9036c8c4..29bfae37686 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/P2TargetUtils.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/P2TargetUtils.java @@ -810,6 +810,10 @@ public static IQueryResult getIUs(ITargetDefinition target, IP * Gets or creates a lock object for the given profile ID. This ensures that * operations on the same profile (which can be shared by multiple ITargetDefinition * instances) are properly synchronized. + *

+ * Note: Lock objects accumulate in memory but this is acceptable since profile IDs + * are based on stable target file paths and the lock objects are small. + *

* * @param profileId the profile identifier * @return a lock object for the profile @@ -818,6 +822,24 @@ private static Object getProfileLock(String profileId) { return PROFILE_LOCKS.computeIfAbsent(profileId, k -> new Object()); } + /** + * Refreshes a profile from the registry after operations that modify it. + * The P2 engine creates new profile snapshots with updated timestamps during + * provisioning operations, so the in-memory profile reference must be refreshed. + * + * @param profile the current profile reference (may have outdated timestamp) + * @param target the target definition + * @return the refreshed profile from the registry + * @throws CoreException if the profile was removed or cannot be loaded + */ + private IProfile refreshProfile(IProfile profile, ITargetDefinition target) throws CoreException { + IProfile refreshed = getProfileRegistry().getProfile(profile.getProfileId()); + if (refreshed == null) { + throw new CoreException(Status.error("Profile was removed: " + getProfileId(target))); //$NON-NLS-1$ + } + return refreshed; + } + /** * Synchronize the profile and the target definition managed by this synchronizer. On return the profile will * be resolved and correctly match the given target. The IUBundleContainers associated with @@ -878,11 +900,7 @@ private void synchronizeInternal(ITargetDefinition target, IProgressMonitor moni resolveWithSlicer(target, profile, progress.split(60)); } // Refresh the profile from registry as resolve operations may have updated it - profile = getProfileRegistry().getProfile(profile.getProfileId()); - if (profile == null) { - throw new CoreException( - Status.error("Profile was removed after resolving: " + getProfileId(target))); //$NON-NLS-1$ - } + profile = refreshProfile(profile, target); fProfile = profile; // If we are updating a profile then delete the old snapshot on success. notify(target, progress.split(15)); @@ -1224,11 +1242,7 @@ public IQueryable getMetadata(IProgressMonitor monitor) { // Refresh the profile from the registry after perform, as the perform operation // creates a new profile snapshot with an updated timestamp - profile = getProfileRegistry().getProfile(profile.getProfileId()); - if (profile == null) { - throw new CoreException( - Status.error("Profile was removed after provisioning: " + getProfileId(target))); //$NON-NLS-1$ - } + profile = refreshProfile(profile, target); // Now that we have a plan with all the binary and explicit bundles, do a second pass and add // in all the source. From c672ff447d5bf374eb86a8f512d1a14b96d0a242 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 5 Nov 2025 07:02:55 +0000 Subject: [PATCH 4/4] Address additional code review feedback - Enhanced documentation for PROFILE_LOCKS explaining lifecycle and memory characteristics - Clarified two-level synchronization strategy (instance + profile level) - Made profile ID retrieval consistent in refreshProfile method - Improved javadoc for better code maintainability Co-authored-by: laeubi <1331477+laeubi@users.noreply.github.com> --- .../internal/core/target/P2TargetUtils.java | 28 ++++++++++++++----- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/P2TargetUtils.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/P2TargetUtils.java index 29bfae37686..1f80dc8e3eb 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/P2TargetUtils.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/P2TargetUtils.java @@ -199,6 +199,15 @@ public class P2TargetUtils { * Locks for profile operations, keyed by profile ID. This ensures that operations * on the same profile (which can be shared by multiple ITargetDefinition instances) * are properly synchronized to prevent timestamp conflicts. + *

+ * Lock objects persist for the lifetime of the application, which is acceptable because: + *

    + *
  • Profile IDs are based on stable target file paths
    • + *
  • The number of unique target files used in a session is typically small
    • + *
  • Lock objects have minimal memory footprint
    • + *
  • Removing locks would introduce complexity and potential race conditions
    • + *
+ *

*/ private static final ConcurrentHashMap PROFILE_LOCKS = new ConcurrentHashMap<>(); @@ -828,14 +837,15 @@ private static Object getProfileLock(String profileId) { * provisioning operations, so the in-memory profile reference must be refreshed. * * @param profile the current profile reference (may have outdated timestamp) - * @param target the target definition + * @param target the target definition (used only for error messages) * @return the refreshed profile from the registry * @throws CoreException if the profile was removed or cannot be loaded */ private IProfile refreshProfile(IProfile profile, ITargetDefinition target) throws CoreException { - IProfile refreshed = getProfileRegistry().getProfile(profile.getProfileId()); + String profileId = profile.getProfileId(); + IProfile refreshed = getProfileRegistry().getProfile(profileId); if (refreshed == null) { - throw new CoreException(Status.error("Profile was removed: " + getProfileId(target))); //$NON-NLS-1$ + throw new CoreException(Status.error("Profile was removed: " + profileId)); //$NON-NLS-1$ } return refreshed; } @@ -848,9 +858,13 @@ private IProfile refreshProfile(IProfile profile, ITargetDefinition target) thro * * NOTE: this is a potentially *very* heavyweight operation. * - * NOTE: this method is synchronized as it is effectively a "test and set" caching method. Two - * threads getting the profile at the same time should not execute concurrently or the profiles - * will get out of sync. + * NOTE: this method uses two levels of synchronization: + *
    + *
  • Instance-level (synchronized method): Protects this P2TargetUtils instance's state (fProfile, etc.)
    • + *
  • Profile-level (profile ID lock): Prevents concurrent access to the same P2 profile from different + * P2TargetUtils instances, which can occur when multiple ITargetDefinition objects refer to the same + * target file and thus share a profile ID
    • + *
* * @throws CoreException if there was a problem synchronizing */ @@ -865,7 +879,7 @@ public synchronized void synchronize(ITargetDefinition target, IProgressMonitor } /** - * Internal implementation of synchronize that runs within the profile lock. + * Internal implementation of synchronize that runs within both instance and profile locks. */ private void synchronizeInternal(ITargetDefinition target, IProgressMonitor monitor) throws CoreException { SubMonitor progress = SubMonitor.convert(monitor, 100);