Do not expose unstarted service, stop started service leak

bundles/org.eclipse.equinox.p2.core/src/org/eclipse/equinox/internal/p2/core/ProvisioningAgent.java

@@ -13,6 +13,8 @@
  *******************************************************************************/
 package org.eclipse.equinox.internal.p2.core;
 
+import static java.lang.String.format;
+
 import java.net.URI;
 import java.util.*;
 import org.eclipse.equinox.p2.core.IAgentLocation;
@@ -95,10 +97,19 @@ private void checkRunning() {
 	@Override
 	public void registerService(String serviceName, Object service) {
 		checkRunning();
-		agentServices.put(serviceName, service);
 		if (service instanceof IAgentService) {
 			((IAgentService) service).start();
 		}
+		if (agentServices.put(serviceName, service) instanceof IAgentService prevService) {
+			if (prevService == service) {
+				agentServices.remove(serviceName, prevService);
+				prevService.stop();
+				throw new IllegalStateException(format(
+						"Service %s for name %s has been registered twice. Double activation of service has happened, service is uregistered.", //$NON-NLS-1$
+						service.getClass().getName(), serviceName));
+			}
+			prevService.stop();
+		}
 	}
 
 	public void setBundleContext(BundleContext context) {

bundles/org.eclipse.equinox.p2.core/src/org/eclipse/equinox/p2/core/IProvisioningAgent.java

@@ -91,8 +91,10 @@ default <T> T getService(Class<T> key) {
 	 * Registers a service with this provisioning agent.
 	 *
 	 * @param serviceName The name of the service to register
-	 * @param service The service implementation
-	 * @exception IllegalStateException if this agent has been stopped
+	 * @param service     The service implementation
+	 * @exception IllegalStateException if this agent has been stopped or service
+	 *                                  has already been registered. The state of
+	 *                                  this service after exception is not defined.
 	 */
 	void registerService(String serviceName, Object service);
 

bundles/org.eclipse.equinox.p2.tests/META-INF/MANIFEST.MF

@@ -130,7 +130,8 @@ Require-Bundle: org.eclipse.equinox.frameworkadmin,
  org.eclipse.equinox.p2.ui.sdk.scheduler,
  org.eclipse.equinox.p2.artifact.repository;bundle-version="[1.3.0,2.0.0)",
  org.mockito.mockito-core,
- net.bytebuddy.byte-buddy
+ net.bytebuddy.byte-buddy,
+ org.eclipse.equinox.p2.core
 Eclipse-RegisterBuddy: org.eclipse.equinox.p2.artifact.repository
 Bundle-RequiredExecutionEnvironment: JavaSE-17
 Eclipse-BundleShape: dir

bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/core/ProvisioningAgentTest.java

@@ -16,9 +16,11 @@
 
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.util.concurrent.CompletableFuture;
 import org.eclipse.equinox.p2.core.IProvisioningAgent;
 import org.eclipse.equinox.p2.core.IProvisioningAgentProvider;
 import org.eclipse.equinox.p2.core.ProvisionException;
+import org.eclipse.equinox.p2.core.spi.IAgentService;
 import org.eclipse.equinox.p2.engine.IProfileRegistry;
 import org.eclipse.equinox.p2.repository.metadata.IMetadataRepositoryManager;
 import org.eclipse.equinox.p2.tests.AbstractProvisioningTest;
@@ -60,4 +62,91 @@ public void testMultipleAgents() throws ProvisionException, URISyntaxException {
 		TestActivator.context.ungetService(providerRef);
 
 	}
+
+	/**
+	 * Stop all registered services
+	 */
+	public void testStopServices() throws ProvisionException {
+		URI p2location = getTempFolder().toURI();
+
+		ServiceReference<IProvisioningAgentProvider> providerRef = TestActivator.context.getServiceReference(IProvisioningAgentProvider.class);
+		IProvisioningAgentProvider provider = TestActivator.context.getService(providerRef);
+
+		IProvisioningAgent firstAgent = provider.createAgent(p2location);
+
+
+		MockService mockService1 = new MockService();
+		MockService mockService2 = new MockService();
+
+		firstAgent.registerService(IMockService.class.getName(), mockService1);
+		assertTrue(mockService1.started);
+
+		firstAgent.registerService(IMockService.class.getName(), mockService2);
+		assertTrue(mockService2.started);
+		assertFalse(mockService1.started);
+
+		firstAgent.stop();
+
+		assertFalse(mockService1.started);
+		assertFalse(mockService2.started);
+
+	}
+
+	/**
+	 * Do not expose unstarted services
+	 */
+	public void testStartServices() throws ProvisionException {
+		URI p2location = getTempFolder().toURI();
+
+		ServiceReference<IProvisioningAgentProvider> providerRef = TestActivator.context
+				.getServiceReference(IProvisioningAgentProvider.class);
+		IProvisioningAgentProvider provider = TestActivator.context.getService(providerRef);
+
+		IProvisioningAgent firstAgent = provider.createAgent(p2location);
+
+
+		CompletableFuture<Void> startStopServiceTask = CompletableFuture.runAsync(() -> {
+			while (!Thread.interrupted()) {
+				if (firstAgent.getService(IMockService.class) == null) {
+					MockService mockService1 = new MockService();
+					firstAgent.registerService(IMockService.class.getName(), mockService1);
+				}
+			}
+
+		});
+
+		long stop = System.currentTimeMillis() + 1000;
+		try {
+			while (System.currentTimeMillis() < stop) {
+				IMockService service = firstAgent.getService(IMockService.class);
+				if (service == null) {
+					continue;
+				}
+				assertTrue(((MockService) service).started);
+				firstAgent.unregisterService(IMockService.class.getName(), service);
+			}
+
+		} finally {
+			startStopServiceTask.cancel(true);
+			firstAgent.stop();
+		}
+	}
+
+	public static interface IMockService extends IAgentService {
+
+	}
+
+	private static class MockService implements IMockService {
+		boolean started = false;
+
+		@Override
+		public void start() {
+			started = true;
+		}
+
+		@Override
+		public void stop() {
+			started = false;
+		}
+	}
 }