Bug 306668 - IProvisioningAgent.getService is NOT thread safe

1 files changed, 40 insertions, 37 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 938e11f97..20df0d9a5 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
@@ -27,7 +27,7 @@ public class ProvisioningAgent implements IProvisioningAgent, ServiceTrackerCust
 
 	private final Map<String, Object> agentServices = Collections.synchronizedMap(new HashMap<String, Object>());
 	private BundleContext context;
-	private boolean stopped = false;
+	private volatile boolean stopped = false;
 	private ServiceRegistration reg;
 	private final Map<ServiceReference, ServiceTracker> trackers = Collections.synchronizedMap(new HashMap<ServiceReference, ServiceTracker>());
 
@@ -44,39 +44,42 @@ public class ProvisioningAgent implements IProvisioningAgent, ServiceTrackerCust
 	 * @see org.eclipse.equinox.p2.core.IProvisioningAgent#getService(java.lang.String)
 	 */
 	public Object getService(String serviceName) {
-		checkRunning();
-		Object service = agentServices.get(serviceName);
-		if (service != null)
+		//synchronize so concurrent gets always obtain the same service
+		synchronized (agentServices) {
+			checkRunning();
+			Object service = agentServices.get(serviceName);
+			if (service != null)
+				return service;
+			//attempt to get factory service from service registry
+			ServiceReference[] refs;
+			try {
+				refs = context.getServiceReferences(IAgentServiceFactory.SERVICE_NAME, "(" + IAgentServiceFactory.PROP_CREATED_SERVICE_NAME + '=' + serviceName + ')'); //$NON-NLS-1$
+			} catch (InvalidSyntaxException e) {
+				e.printStackTrace();
+				return null;
+			}
+			if (refs == null || refs.length == 0)
+				return null;
+			//track the factory so that we can automatically remove the service when the factory goes away
+			ServiceTracker tracker = new ServiceTracker(context, refs[0], 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(refs[0], tracker);
 			return service;
-		//attempt to get factory service from service registry
-		ServiceReference[] refs;
-		try {
-			refs = context.getServiceReferences(IAgentServiceFactory.SERVICE_NAME, "(" + IAgentServiceFactory.PROP_CREATED_SERVICE_NAME + '=' + serviceName + ')'); //$NON-NLS-1$
-		} catch (InvalidSyntaxException e) {
-			e.printStackTrace();
-			return null;
-		}
-		if (refs == null || refs.length == 0)
-			return null;
-		//track the factory so that we can automatically remove the service when the factory goes away
-		ServiceTracker tracker = new ServiceTracker(context, refs[0], 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(refs[0], tracker);
-		return service;
 	}
 
-	private synchronized void checkRunning() {
+	private void checkRunning() {
 		if (stopped)
 			throw new IllegalStateException("Attempt to access stopped agent: " + this); //$NON-NLS-1$
 	}
@@ -108,11 +111,9 @@ public class ProvisioningAgent implements IProvisioningAgent, ServiceTrackerCust
 	}
 
 	public void unregisterService(String serviceName, Object service) {
-		synchronized (this) {
+		synchronized (agentServices) {
 			if (stopped)
 				return;
-		}
-		synchronized (agentServices) {
 			if (agentServices.get(serviceName) == service)
 				agentServices.remove(serviceName);
 		}
@@ -121,15 +122,17 @@ public class ProvisioningAgent implements IProvisioningAgent, ServiceTrackerCust
 	}
 
 	public void stop() {
+		List<Object> toStop;
+		synchronized (agentServices) {
+			toStop = new ArrayList<Object>(agentServices.values());
+		}
 		//give services a chance to do their own shutdown
-		for (Object service : agentServices.values()) {
+		for (Object service : toStop) {
 			if (service instanceof IAgentService)
 				if (service != this)
 					((IAgentService) service).stop();
 		}
-		synchronized (this) {
-			stopped = true;
-		}
+		stopped = true;
 		//close all service trackers
 		synchronized (trackers) {
 			for (ServiceTracker t : trackers.values())