From 0e76fafd32e3b6429549bb1ca97c666adfdec64f Mon Sep 17 00:00:00 2001 From: Vasili Gulevich Date: Sun, 31 Aug 2025 20:06:07 +0400 Subject: [PATCH 1/4] Fix deadlock #937 The deadlock was caused by working with ServiceTracker (which may activate bundles) while holding a lock Do not hold any locks while working with ServiceTracker Fixes #937 --- .../internal/p2/core/ProvisioningAgent.java | 118 ++++++------------ 1 file changed, 41 insertions(+), 77 deletions(-) diff --git a/bundles/org.eclipse.equinox.p2.core/src/org/eclipse/equinox/internal/p2/core/ProvisioningAgent.java b/bundles/org.eclipse.equinox.p2.core/src/org/eclipse/equinox/internal/p2/core/ProvisioningAgent.java index d84dea35b7..a5cb1ce9df 100644 --- a/bundles/org.eclipse.equinox.p2.core/src/org/eclipse/equinox/internal/p2/core/ProvisioningAgent.java +++ b/bundles/org.eclipse.equinox.p2.core/src/org/eclipse/equinox/internal/p2/core/ProvisioningAgent.java @@ -28,13 +28,13 @@ /** * Represents a p2 agent instance. */ -public class ProvisioningAgent implements IProvisioningAgent, ServiceTrackerCustomizer { +public class ProvisioningAgent implements IProvisioningAgent { private final Map agentServices = Collections.synchronizedMap(new HashMap<>()); private BundleContext context; private volatile boolean stopped = false; private ServiceRegistration reg; - private final Map, ServiceTracker> trackers = Collections + private final Map> trackers = Collections .synchronizedMap(new HashMap<>()); /** @@ -55,37 +55,22 @@ public Object getService(String serviceName) { if (service != null) { return service; } - //attempt to get factory service from service registry - Collection> refs; - try { - refs = context.getServiceReferences(IAgentServiceFactory.class, - String.format("(|(%s=%s)(p2.agent.servicename=%s))", //$NON-NLS-1$ use old property as fallback - IAgentServiceFactory.PROP_AGENT_SERVICE_NAME, serviceName, serviceName)); - } catch (InvalidSyntaxException e) { - e.printStackTrace(); - return null; - } - if (refs == null || refs.isEmpty()) { - return null; - } - ServiceReference firstRef = Collections.max(refs); - //track the factory so that we can automatically remove the service when the factory goes away - ServiceTracker tracker = new ServiceTracker<>(context, firstRef, this); - tracker.open(); - IAgentServiceFactory factory = (IAgentServiceFactory) tracker.getService(); - if (factory == null) { - tracker.close(); - return null; - } - service = factory.createService(this); - if (service == null) { - tracker.close(); - return null; - } - registerService(serviceName, service); - trackers.put(firstRef, tracker); - return service; } + ServiceTracker tracker = trackers.computeIfAbsent(serviceName, + ref -> { + try { + Filter filter = context.createFilter( + String.format("(&(%s=%s)(|(%s=%s)(p2.agent.servicename=%s))", //$NON-NLS-1$ + Constants.OBJECTCLASS, IAgentServiceFactory.class.getName(), // + IAgentServiceFactory.PROP_AGENT_SERVICE_NAME, serviceName, // + serviceName)); // use old property as fallback + return new ServiceTracker<>(context, filter, trackerCustomizer); + } catch (InvalidSyntaxException e) { + throw new AssertionError(e); + } + }); + tracker.open(); + return tracker.getService(); } private void checkRunning() { @@ -133,14 +118,10 @@ public void setLocation(URI location) { @Override public void unregisterService(String serviceName, Object service) { - synchronized (agentServices) { - if (stopped) { - return; - } - if (agentServices.get(serviceName) == service) { - agentServices.remove(serviceName); - } + if (stopped) { + return; } + agentServices.remove(serviceName, service); if (service instanceof IAgentService) { ((IAgentService) service).stop(); } @@ -178,49 +159,32 @@ public void setServiceRegistration(ServiceRegistration reg) this.reg = reg; } - @Override - public Object addingService(ServiceReference reference) { - if (stopped) { - return null; + private final ServiceTrackerCustomizer trackerCustomizer = new ServiceTrackerCustomizer<>() { + @Override + public Object addingService(ServiceReference reference) { + if (stopped) { + return null; + } + Object result = context.getService(reference).createService(ProvisioningAgent.this); + if (result instanceof IAgentService agentService) { + agentService.start(); + } + return result; } - return context.getService(reference); - } - @Override - public void modifiedService(ServiceReference reference, Object service) { - //nothing to do - } - - @Override - public void removedService(ServiceReference reference, Object factoryService) { - if (stopped) { - return; + @Override + public void modifiedService(ServiceReference reference, Object service) { + // nothing to do } - String serviceName = getAgentServiceName(reference); - if (serviceName == null) { - return; - } - Object registered = agentServices.get(serviceName); - if (registered == null) { - return; - } - if (FrameworkUtil.getBundle(registered.getClass()) == FrameworkUtil.getBundle(factoryService.getClass())) { - //the service we are holding is going away - unregisterService(serviceName, registered); - ServiceTracker toRemove = trackers.remove(reference); - if (toRemove != null) { - toRemove.close(); + + @Override + public void removedService(ServiceReference reference, Object service) { + if (service instanceof IAgentService agentService) { + agentService.stop(); } } - } - private String getAgentServiceName(ServiceReference reference) { - Object property = reference.getProperty(IAgentServiceFactory.PROP_AGENT_SERVICE_NAME); - if (property instanceof String s) { - return s; - } - // backward compatibility - return (String) reference.getProperty("p2.agent.servicename"); //$NON-NLS-1$ - } + }; + } From 032ddd008e9356205170b2b24bfe3c17eb51f0f9 Mon Sep 17 00:00:00 2001 From: Vasili Gulevich Date: Mon, 1 Sep 2025 00:23:44 +0400 Subject: [PATCH 2/4] Do not leak a reference to removed service #937 Re-implement overridden default removedService() --- .../org/eclipse/equinox/internal/p2/core/ProvisioningAgent.java | 1 + 1 file changed, 1 insertion(+) diff --git a/bundles/org.eclipse.equinox.p2.core/src/org/eclipse/equinox/internal/p2/core/ProvisioningAgent.java b/bundles/org.eclipse.equinox.p2.core/src/org/eclipse/equinox/internal/p2/core/ProvisioningAgent.java index a5cb1ce9df..6ef6fc0cb9 100644 --- a/bundles/org.eclipse.equinox.p2.core/src/org/eclipse/equinox/internal/p2/core/ProvisioningAgent.java +++ b/bundles/org.eclipse.equinox.p2.core/src/org/eclipse/equinox/internal/p2/core/ProvisioningAgent.java @@ -182,6 +182,7 @@ public void removedService(ServiceReference reference, Obj if (service instanceof IAgentService agentService) { agentService.stop(); } + context.ungetService(reference); } }; From 8fb5d1df34554e0c453fd354f54a1463e4bcffbb Mon Sep 17 00:00:00 2001 From: Vasili Gulevich Date: Mon, 1 Sep 2025 00:34:07 +0400 Subject: [PATCH 3/4] Fix typo #937 --- .../org/eclipse/equinox/internal/p2/core/ProvisioningAgent.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundles/org.eclipse.equinox.p2.core/src/org/eclipse/equinox/internal/p2/core/ProvisioningAgent.java b/bundles/org.eclipse.equinox.p2.core/src/org/eclipse/equinox/internal/p2/core/ProvisioningAgent.java index 6ef6fc0cb9..9353ca5d15 100644 --- a/bundles/org.eclipse.equinox.p2.core/src/org/eclipse/equinox/internal/p2/core/ProvisioningAgent.java +++ b/bundles/org.eclipse.equinox.p2.core/src/org/eclipse/equinox/internal/p2/core/ProvisioningAgent.java @@ -60,7 +60,7 @@ public Object getService(String serviceName) { ref -> { try { Filter filter = context.createFilter( - String.format("(&(%s=%s)(|(%s=%s)(p2.agent.servicename=%s))", //$NON-NLS-1$ + String.format("(&(%s=%s)(|(%s=%s)(p2.agent.servicename=%s)))", //$NON-NLS-1$ Constants.OBJECTCLASS, IAgentServiceFactory.class.getName(), // IAgentServiceFactory.PROP_AGENT_SERVICE_NAME, serviceName, // serviceName)); // use old property as fallback From e065a303e243fcb7fd16e6a80c21bc6b35c6fa96 Mon Sep 17 00:00:00 2001 From: Vasili Gulevich Date: Mon, 1 Sep 2025 15:34:41 +0400 Subject: [PATCH 4/4] Allow stop procedure to access still running services #937 --- .../internal/p2/core/ProvisioningAgent.java | 92 +++++++++++++------ 1 file changed, 64 insertions(+), 28 deletions(-) diff --git a/bundles/org.eclipse.equinox.p2.core/src/org/eclipse/equinox/internal/p2/core/ProvisioningAgent.java b/bundles/org.eclipse.equinox.p2.core/src/org/eclipse/equinox/internal/p2/core/ProvisioningAgent.java index 9353ca5d15..064a4d2231 100644 --- a/bundles/org.eclipse.equinox.p2.core/src/org/eclipse/equinox/internal/p2/core/ProvisioningAgent.java +++ b/bundles/org.eclipse.equinox.p2.core/src/org/eclipse/equinox/internal/p2/core/ProvisioningAgent.java @@ -17,6 +17,7 @@ import java.net.URI; import java.util.*; +import java.util.function.Consumer; import org.eclipse.equinox.p2.core.IAgentLocation; import org.eclipse.equinox.p2.core.IProvisioningAgent; import org.eclipse.equinox.p2.core.spi.IAgentService; @@ -30,12 +31,12 @@ */ public class ProvisioningAgent implements IProvisioningAgent { - private final Map agentServices = Collections.synchronizedMap(new HashMap<>()); + private final Map agentServices = Collections.synchronizedMap(new LinkedHashMap<>()); private BundleContext context; private volatile boolean stopped = false; private ServiceRegistration reg; private final Map> trackers = Collections - .synchronizedMap(new HashMap<>()); + .synchronizedMap(new LinkedHashMap<>()); /** * Instantiates a provisioning agent. @@ -50,7 +51,6 @@ public ProvisioningAgent() { public Object getService(String serviceName) { //synchronize so concurrent gets always obtain the same service synchronized (agentServices) { - checkRunning(); Object service = agentServices.get(serviceName); if (service != null) { return service; @@ -59,6 +59,9 @@ public Object getService(String serviceName) { ServiceTracker tracker = trackers.computeIfAbsent(serviceName, ref -> { try { + if (stopped) { + return null; + } Filter filter = context.createFilter( String.format("(&(%s=%s)(|(%s=%s)(p2.agent.servicename=%s)))", //$NON-NLS-1$ Constants.OBJECTCLASS, IAgentServiceFactory.class.getName(), // @@ -69,6 +72,9 @@ public Object getService(String serviceName) { throw new AssertionError(e); } }); + if (tracker == null) { + return null; + } tracker.open(); return tracker.getService(); } @@ -118,9 +124,6 @@ public void setLocation(URI location) { @Override public void unregisterService(String serviceName, Object service) { - if (stopped) { - return; - } agentServices.remove(serviceName, service); if (service instanceof IAgentService) { ((IAgentService) service).stop(); @@ -129,29 +132,20 @@ public void unregisterService(String serviceName, Object service) { @Override public void stop() { - List toStop; - synchronized (agentServices) { - toStop = new ArrayList<>(agentServices.values()); - } - //give services a chance to do their own shutdown - for (Object service : toStop) { - if (service instanceof IAgentService) { - if (service != this) { - ((IAgentService) service).stop(); - } - } - } stopped = true; - //close all service trackers - synchronized (trackers) { - for (ServiceTracker t : trackers.values()) { - t.close(); + try { + disposeInTurns(agentServices.entrySet(), entry -> { + unregisterService(entry.getKey(), entry.getValue()); + }); + } finally { + try { + disposeInTurns(trackers.values(), ServiceTracker::close); + } finally { + if (reg != null) { + reg.unregister(); + reg = null; + } } - trackers.clear(); - } - if (reg != null) { - reg.unregister(); - reg = null; } } @@ -182,10 +176,52 @@ public void removedService(ServiceReference reference, Obj if (service instanceof IAgentService agentService) { agentService.stop(); } - context.ungetService(reference); + if (service != null) { + context.ungetService(reference); + } } }; + /** + * Given a thread-safe collection, drain all elements one-by-one, disposing each + * without holding locks and allowing concurrent modification form. Other + * threads should not attempt to add elements to collection.
+ * Usage example: + * + * 
{@code
+	 * volatile boolean stopped = false;
+	 * ...
+	 * stopped = true; // prevent other threads from adding to the collection
+	 * disposeInTurns(agentServices.entrySet(), entry -> {
+	 * 	unregisterService(entry.getKey(), entry.getValue());
+	 * });
+	 * }
+ * + */ + @SuppressWarnings("unchecked") + private void disposeInTurns(Collection input, Consumer dispose) { + RuntimeException error = null; + while (!input.isEmpty()) { + Object[] toStop = input.toArray(Object[]::new); + Collections.reverse(Arrays.asList(toStop)); // Remove in an inverse order of adding + for (Object item : toStop) { + if (input.remove(item)) { + try { + dispose.accept((T) item); + } catch (RuntimeException e) { + if (error == null) { + error = e; + } else { + error.addSuppressed(e); + } + } + } + } + } + if (error != null) { + throw error; + } + } }