Bug 487842 - Unable to acquire state change lock for the module when

starting a bundle

Must increment inStart BEFORE acquiring the STARTED state the first
time.  There is still an unavoidable timing issue.  The window is now
as small as possible.  A new flag is added to disable auto-starting
newly resolved bundles that are marked for persistent start.

Also fixed bug in how modules are restarted after a refresh.
They must use their activation policy if it is set.

Change-Id: I8175af9ed29e611659a8b8bc0b64b0a3874e68bf
Signed-off-by: Thomas Watson <tjwatson@us.ibm.com>

7 files changed, 78 insertions, 30 deletions

diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/ClassLoadingBundleTests.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/ClassLoadingBundleTests.java
index 57d1bd811..1f623a1fe 100644
--- a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/ClassLoadingBundleTests.java
+++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/ClassLoadingBundleTests.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2006, 2015 IBM Corporation and others.
+ * Copyright (c) 2006, 2016 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
  * which accompanies this distribution, and is available at
@@ -98,7 +98,7 @@ public class ClassLoadingBundleTests extends AbstractBundleTests {
 		Bundle chainTestD = installer.installBundle("chain.test.d"); //$NON-NLS-1$
 		chainTest.loadClass("chain.test.TestMultiChain").newInstance(); //$NON-NLS-1$
 
-		Object[] expectedEvents = new Object[12];
+		Object[] expectedEvents = new Object[8];
 		expectedEvents[0] = new BundleEvent(BundleEvent.STARTED, chainTestD);
 		expectedEvents[1] = new BundleEvent(BundleEvent.STARTED, chainTestB);
 		expectedEvents[2] = new BundleEvent(BundleEvent.STARTED, chainTestC);
@@ -107,14 +107,20 @@ public class ClassLoadingBundleTests extends AbstractBundleTests {
 		expectedEvents[5] = new BundleEvent(BundleEvent.STOPPED, chainTestB);
 		expectedEvents[6] = new BundleEvent(BundleEvent.STOPPED, chainTestC);
 		expectedEvents[7] = new BundleEvent(BundleEvent.STOPPED, chainTestD);
-		expectedEvents[8] = new BundleEvent(BundleEvent.STARTED, chainTestD);
-		expectedEvents[9] = new BundleEvent(BundleEvent.STARTED, chainTestC);
-		expectedEvents[10] = new BundleEvent(BundleEvent.STARTED, chainTestB);
-		expectedEvents[11] = new BundleEvent(BundleEvent.STARTED, chainTestA);
 
 		installer.refreshPackages(new Bundle[] {chainTestC, chainTestD});
 
-		Object[] actualEvents = simpleResults.getResults(12);
+		Object[] actualEvents = simpleResults.getResults(8);
+		compareResults(expectedEvents, actualEvents);
+
+		chainTest.loadClass("chain.test.TestMultiChain").newInstance(); //$NON-NLS-1$
+		expectedEvents = new Object[4];
+		expectedEvents[0] = new BundleEvent(BundleEvent.STARTED, chainTestD);
+		expectedEvents[1] = new BundleEvent(BundleEvent.STARTED, chainTestB);
+		expectedEvents[2] = new BundleEvent(BundleEvent.STARTED, chainTestC);
+		expectedEvents[3] = new BundleEvent(BundleEvent.STARTED, chainTestA);
+
+		actualEvents = simpleResults.getResults(4);
 		compareResults(expectedEvents, actualEvents);
 	}
 
diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/TestModuleContainer.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/TestModuleContainer.java
index efb7eda25..90b6cad05 100644
--- a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/TestModuleContainer.java
+++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/TestModuleContainer.java
@@ -988,7 +988,7 @@ public class TestModuleContainer extends AbstractTest {
 		container.refresh(Arrays.asList(lazy1));
 
 		actual = database.getModuleEvents();
-		expected = new ArrayList<DummyModuleEvent>(Arrays.asList(new DummyModuleEvent(lazy1, ModuleEvent.STOPPING, State.STOPPING), new DummyModuleEvent(lazy1, ModuleEvent.STOPPED, State.RESOLVED), new DummyModuleEvent(lazy1, ModuleEvent.UNRESOLVED, State.INSTALLED), new DummyModuleEvent(lazy1, ModuleEvent.RESOLVED, State.RESOLVED), new DummyModuleEvent(lazy1, ModuleEvent.STARTING, State.STARTING), new DummyModuleEvent(lazy1, ModuleEvent.STARTED, State.ACTIVE)));
+		expected = new ArrayList<DummyModuleEvent>(Arrays.asList(new DummyModuleEvent(lazy1, ModuleEvent.STOPPING, State.STOPPING), new DummyModuleEvent(lazy1, ModuleEvent.STOPPED, State.RESOLVED), new DummyModuleEvent(lazy1, ModuleEvent.UNRESOLVED, State.INSTALLED), new DummyModuleEvent(lazy1, ModuleEvent.RESOLVED, State.RESOLVED), new DummyModuleEvent(lazy1, ModuleEvent.LAZY_ACTIVATION, State.LAZY_STARTING)));
 		assertEvents(expected, actual, true);
 
 		container.update(lazy1, OSGiManifestBuilderFactory.createBuilder(getManifest("lazy1_v1.MF")), null);
@@ -2369,7 +2369,20 @@ public class TestModuleContainer extends AbstractTest {
 
 	@Test
 	public void testStartOnResolve() throws BundleException, IOException {
-		DummyContainerAdaptor adaptor = createDummyAdaptor();
+		doTestStartOnResolve(true);
+	}
+
+	@Test
+	public void testDisableStartOnResolve() throws BundleException, IOException {
+		doTestStartOnResolve(false);
+	}
+
+	private void doTestStartOnResolve(boolean enabled) throws BundleException, IOException {
+		Map<String, String> configuration = new HashMap<String, String>();
+		if (!enabled) {
+			configuration.put(EquinoxConfiguration.PROP_MODULE_AUTO_START_ON_RESOLVE, Boolean.toString(false));
+		}
+		DummyContainerAdaptor adaptor = new DummyContainerAdaptor(new DummyCollisionHook(false), configuration);
 		ModuleContainer container = adaptor.getContainer();
 
 		// install the system.bundle
@@ -2405,8 +2418,9 @@ public class TestModuleContainer extends AbstractTest {
 		report = container.resolve(Collections.<Module> emptySet(), false);
 		Assert.assertNull("Found a error.", report.getResolutionException());
 
+		State expectedState = enabled ? State.ACTIVE : State.RESOLVED;
 		for (Module module : modules) {
-			Assert.assertEquals("Wrong state.", State.ACTIVE, module.getState());
+			Assert.assertEquals("Wrong state.", expectedState, module.getState());
 		}
 	}
 
diff --git a/bundles/org.eclipse.osgi/.gitignore b/bundles/org.eclipse.osgi/.gitignore
index 7d67e02a9..bb575b389 100644
--- a/bundles/org.eclipse.osgi/.gitignore
+++ b/bundles/org.eclipse.osgi/.gitignore
@@ -8,3 +8,4 @@ eclipseAdaptor.jar
 resolver.jar
 /generated/
 .DS_Store
+/configuration/
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/Module.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/Module.java
index 7970b4f8b..39c2ba26b 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/Module.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/Module.java
@@ -161,7 +161,7 @@ public abstract class Module implements BundleReference, BundleStartLevel, Compa
 	final EquinoxReentrantLock stateChangeLock = new EquinoxReentrantLock();
 	private final EnumSet<ModuleEvent> stateTransitionEvents = EnumSet.noneOf(ModuleEvent.class);
 	private final EnumSet<Settings> settings;
-	private final AtomicInteger inStartResolve = new AtomicInteger(0);
+	final AtomicInteger inStart = new AtomicInteger(0);
 	private volatile State state = State.INSTALLED;
 	private volatile int startlevel;
 	private volatile long lastModified;
@@ -394,22 +394,25 @@ public abstract class Module implements BundleReference, BundleStartLevel, Compa
 		}
 		BundleException startError = null;
 		boolean lockedStarted = false;
-		lockStateChange(ModuleEvent.STARTED);
+		// Indicate we are in the middle of a start.
+		// This must be incremented before we acquire the STARTED lock the first time.
+		inStart.incrementAndGet();
 		try {
-			inStartResolve.incrementAndGet();
+			lockStateChange(ModuleEvent.STARTED);
 			lockedStarted = true;
 			checkValid();
 			if (StartOptions.TRANSIENT_IF_AUTO_START.isContained(options) && !settings.contains(Settings.AUTO_START)) {
-				// Do nothing
+				// Do nothing; this is a request to start only if the module is set for auto start
 				return;
 			}
 			checkFragment();
 			persistStartOptions(options);
 			if (getStartLevel() > getRevisions().getContainer().getStartLevel()) {
 				if (StartOptions.TRANSIENT.isContained(options)) {
+					// it is an error to attempt to transient start a bundle without its start level met
 					throw new BundleException(Msg.Module_Transient_StartError, BundleException.START_TRANSIENT_ERROR);
 				}
-				// DO nothing
+				// Do nothing; start level is not met
 				return;
 			}
 			if (State.ACTIVE.equals(getState()))
@@ -421,7 +424,6 @@ public abstract class Module implements BundleReference, BundleStartLevel, Compa
 				unlockStateChange(ModuleEvent.STARTED);
 				lockedStarted = false;
 				try {
-
 					report = getRevisions().getContainer().resolve(Arrays.asList(this), true);
 				} finally {
 					lockStateChange(ModuleEvent.STARTED);
@@ -456,7 +458,7 @@ public abstract class Module implements BundleReference, BundleStartLevel, Compa
 			if (lockedStarted) {
 				unlockStateChange(ModuleEvent.STARTED);
 			}
-			inStartResolve.decrementAndGet();
+			inStart.decrementAndGet();
 		}
 
 		if (event != null) {
@@ -701,7 +703,12 @@ public abstract class Module implements BundleReference, BundleStartLevel, Compa
 		return current == null ? false : current.hasLazyActivatePolicy();
 	}
 
-	final boolean inStartResolve() {
-		return inStartResolve.get() > 0;
+	/**
+	 * Used internally by the container to determine if any thread is in the middle
+	 * of a start operation on this module.
+	 * @return
+	 */
+	final boolean inStart() {
+		return inStart.get() > 0;
 	}
 }
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleContainer.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleContainer.java
index c7e02ab7c..00c40a0c8 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleContainer.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleContainer.java
@@ -90,6 +90,8 @@ public final class ModuleContainer implements DebugOptionsListener {
 
 	private final long moduleLockTimeout;
 
+	private final boolean autoStartOnResolve;
+
 	boolean DEBUG_MONITOR_LAZY = false;
 
 	/**
@@ -121,6 +123,12 @@ public final class ModuleContainer implements DebugOptionsListener {
 		if (debugOptions != null) {
 			this.DEBUG_MONITOR_LAZY = debugOptions.getBooleanOption(Debug.OPTION_MONITOR_LAZY, false);
 		}
+
+		String autoStartOnResolveProp = adaptor.getProperty(EquinoxConfiguration.PROP_MODULE_AUTO_START_ON_RESOLVE);
+		if (autoStartOnResolveProp == null) {
+			autoStartOnResolveProp = Boolean.toString(true);
+		}
+		this.autoStartOnResolve = Boolean.parseBoolean(autoStartOnResolveProp);
 	}
 
 	/**
@@ -702,15 +710,22 @@ public final class ModuleContainer implements DebugOptionsListener {
 		if (restartTriggers) {
 			for (Module module : triggers) {
 				if (module.getId() != 0 && Module.RESOLVED_SET.contains(module.getState())) {
-					start(module, StartOptions.TRANSIENT);
+					start(module, StartOptions.TRANSIENT_RESUME);
 				}
 			}
 		}
-		// This is questionable behavior according to the spec but this was the way equinox previously behaved
-		// Need to auto-start any persistently started bundles that got resolved
-		for (Module module : modulesLocked) {
-			if (!module.inStartResolve() && module.getId() != 0 && !triggerSet.contains(module)) {
-				start(module, StartOptions.TRANSIENT_IF_AUTO_START, StartOptions.TRANSIENT_RESUME);
+		if (autoStartOnResolve) {
+			// This is questionable behavior according to the spec but this was the way equinox previously behaved
+			// Need to auto-start any persistently started bundles that got resolved
+			for (Module module : modulesLocked) {
+				// Note that we check inStart here.  There is still a timing issue that is impossible to avoid.
+				// Another thread could attempt to start the module but we could check inStart() before that thread
+				// increments inStart.  One thread will win the race to grab the module STARTED lock.  That thread
+				// will end up actually starting the module and the other thread will block.  If a timeout occurs
+				// the blocking thread will get an exception.
+				if (!module.inStart() && module.getId() != 0 && !triggerSet.contains(module)) {
+					start(module, StartOptions.TRANSIENT_IF_AUTO_START, StartOptions.TRANSIENT_RESUME);
+				}
 			}
 		}
 		return true;
@@ -860,8 +875,7 @@ public final class ModuleContainer implements DebugOptionsListener {
 					} catch (BundleException e) {
 						adaptor.publishContainerEvent(ContainerEvent.ERROR, refreshModule, e);
 					}
-				}
-				if (!State.ACTIVE.equals(previousState)) {
+				} else {
 					iTriggers.remove();
 				}
 			}
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/SystemModule.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/SystemModule.java
index 9c586109b..58cd1e93f 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/SystemModule.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/SystemModule.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2012, 2014 IBM Corporation and others.
+ * Copyright (c) 2012, 2016 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
  * which accompanies this distribution, and is available at
@@ -40,11 +40,15 @@ public abstract class SystemModule extends Module {
 	 */
 	public final void init() throws BundleException {
 		getRevisions().getContainer().checkAdminPermission(getBundle(), AdminPermission.EXECUTE);
+
 		boolean lockedStarted = false;
-		lockStateChange(ModuleEvent.STARTED);
+		// Indicate we are in the middle of a start.
+		// This must be incremented before we acquire the STARTED lock the first time.
+		inStart.incrementAndGet();
 		try {
-			getContainer().getAdaptor().initBegin();
+			lockStateChange(ModuleEvent.STARTED);
 			lockedStarted = true;
+			getContainer().getAdaptor().initBegin();
 			checkValid();
 			if (ACTIVE_SET.contains(getState()))
 				return;
@@ -97,6 +101,7 @@ public abstract class SystemModule extends Module {
 			if (lockedStarted) {
 				unlockStateChange(ModuleEvent.STARTED);
 			}
+			inStart.decrementAndGet();
 		}
 	}
 
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/framework/EquinoxConfiguration.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/framework/EquinoxConfiguration.java
index 8000f44db..5c05ac31e 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/framework/EquinoxConfiguration.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/framework/EquinoxConfiguration.java
@@ -192,6 +192,7 @@ public class EquinoxConfiguration implements EnvironmentInfo {
 	public static final String PROP_STATE_SAVE_DELAY_INTERVAL = "eclipse.stateSaveDelayInterval"; //$NON-NLS-1$
 
 	public static final String PROP_MODULE_LOCK_TIMEOUT = "osgi.module.lock.timeout"; //$NON-NLS-1$
+	public static final String PROP_MODULE_AUTO_START_ON_RESOLVE = "osgi.module.auto.start.on.resolve"; //$NON-NLS-1$
 	public static final String PROP_ALLOW_RESTRICTED_PROVIDES = "osgi.equinox.allow.restricted.provides"; //$NON-NLS-1$
 	public static final String PROP_LOG_HISTORY_MAX = "equinox.log.history.max"; //$NON-NLS-1$