Bug 487842 - Unable to acquire state change lock for the module whenI20160216-1400I20160216-0800

starting a bundle

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

4 files changed, 156 insertions, 35 deletions

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 c1a7acb6f..efb7eda25 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
@@ -12,6 +12,7 @@ package org.eclipse.osgi.tests.container;
 
 import static java.util.jar.Attributes.Name.MANIFEST_VERSION;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
 
 import java.io.*;
 import java.util.*;
@@ -2366,6 +2367,101 @@ public class TestModuleContainer extends AbstractTest {
 		}
 	}
 
+	@Test
+	public void testStartOnResolve() throws BundleException, IOException {
+		DummyContainerAdaptor adaptor = createDummyAdaptor();
+		ModuleContainer container = adaptor.getContainer();
+
+		// install the system.bundle
+		Module systemBundle = installDummyModule("system.bundle.MF", Constants.SYSTEM_BUNDLE_LOCATION, Constants.SYSTEM_BUNDLE_SYMBOLICNAME, null, null, container);
+		ResolutionReport report = container.resolve(Arrays.asList(systemBundle), true);
+		Assert.assertNull("Failed to resolve system.bundle.", report.getResolutionException());
+		systemBundle.start();
+
+		// install a bunch of modules
+		Map<String, String> manifest = new HashMap<String, String>();
+		List<Module> modules = new ArrayList<Module>();
+		for (int i = 0; i < 5; i++) {
+			manifest.clear();
+			manifest.put(Constants.BUNDLE_MANIFESTVERSION, "2");
+			manifest.put(Constants.BUNDLE_SYMBOLICNAME, "module." + i);
+			manifest.put(Constants.IMPORT_PACKAGE, "export");
+			Module module = installDummyModule(manifest, manifest.get(Constants.BUNDLE_SYMBOLICNAME), container);
+			try {
+				module.start();
+				fail("expected a bundle exception.");
+			} catch (BundleException e) {
+				// do nothing
+			}
+			modules.add(module);
+		}
+
+		manifest.clear();
+		manifest.put(Constants.BUNDLE_MANIFESTVERSION, "2");
+		manifest.put(Constants.BUNDLE_SYMBOLICNAME, "exporter");
+		manifest.put(Constants.EXPORT_PACKAGE, "export");
+		installDummyModule(manifest, manifest.get(Constants.BUNDLE_SYMBOLICNAME), container);
+
+		report = container.resolve(Collections.<Module> emptySet(), false);
+		Assert.assertNull("Found a error.", report.getResolutionException());
+
+		for (Module module : modules) {
+			Assert.assertEquals("Wrong state.", State.ACTIVE, module.getState());
+		}
+	}
+
+	@Test
+	public void testResolveDeadlock() throws BundleException, IOException, InterruptedException {
+		DummyContainerAdaptor adaptor = createDummyAdaptor();
+		ModuleContainer container = adaptor.getContainer();
+
+		// install the system.bundle
+		Module systemBundle = installDummyModule("system.bundle.MF", Constants.SYSTEM_BUNDLE_LOCATION, Constants.SYSTEM_BUNDLE_SYMBOLICNAME, null, null, container);
+		ResolutionReport report = container.resolve(Arrays.asList(systemBundle), true);
+		Assert.assertNull("Failed to resolve system.bundle.", report.getResolutionException());
+		systemBundle.start();
+
+		// install a bunch of modules
+		Map<String, String> manifest = new HashMap<String, String>();
+		List<Module> modules = new ArrayList<Module>();
+		for (int i = 0; i < 5; i++) {
+			manifest.clear();
+			manifest.put(Constants.BUNDLE_MANIFESTVERSION, "2");
+			manifest.put(Constants.BUNDLE_SYMBOLICNAME, "module." + i);
+			modules.add(installDummyModule(manifest, manifest.get(Constants.BUNDLE_SYMBOLICNAME), container));
+		}
+		adaptor.setSlowdownEvents(true);
+		final ConcurrentLinkedQueue<BundleException> startErrors = new ConcurrentLinkedQueue<BundleException>();
+		final ExecutorService executor = Executors.newFixedThreadPool(10);
+		try {
+			for (final Module module : modules) {
+
+				executor.execute(new Runnable() {
+
+					@Override
+					public void run() {
+						try {
+							module.start();
+						} catch (BundleException e) {
+							startErrors.offer(e);
+							e.printStackTrace();
+						}
+					}
+				});
+			}
+		} finally {
+			executor.shutdown();
+			executor.awaitTermination(5, TimeUnit.MINUTES);
+			systemBundle.stop();
+		}
+
+		Assert.assertNull("Found a error.", startErrors.poll());
+		List<DummyContainerEvent> events = adaptor.getDatabase().getContainerEvents();
+		for (DummyContainerEvent event : events) {
+			Assert.assertNotEquals("Found an error.", ContainerEvent.ERROR, event.type);
+		}
+	}
+
 	private static void assertWires(List<ModuleWire> required, List<ModuleWire>... provided) {
 		for (ModuleWire requiredWire : required) {
 			for (List<ModuleWire> providedList : provided) {
diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/dummys/DummyContainerAdaptor.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/dummys/DummyContainerAdaptor.java
index 645004e35..630b2f43e 100644
--- a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/dummys/DummyContainerAdaptor.java
+++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/dummys/DummyContainerAdaptor.java
@@ -13,6 +13,7 @@ package org.eclipse.osgi.tests.container.dummys;
 import java.util.EnumSet;
 import java.util.Map;
 import java.util.concurrent.Executor;
+import java.util.concurrent.atomic.AtomicBoolean;
 import org.eclipse.osgi.container.*;
 import org.eclipse.osgi.container.Module.Settings;
 import org.eclipse.osgi.service.debug.DebugOptions;
@@ -22,6 +23,7 @@ import org.osgi.framework.FrameworkListener;
 import org.osgi.framework.hooks.resolver.ResolverHookFactory;
 
 public class DummyContainerAdaptor extends ModuleContainerAdaptor {
+	private AtomicBoolean slowdownEvents = new AtomicBoolean(false);
 	private final ModuleCollisionHook collisionHook;
 	private final Map<String, String> configuration;
 	private final DummyModuleDatabase moduleDatabase;
@@ -86,8 +88,20 @@ public class DummyContainerAdaptor extends ModuleContainerAdaptor {
 		return moduleDatabase;
 	}
 
+	public void setSlowdownEvents(boolean slowdown) {
+		slowdownEvents.set(slowdown);
+	}
+
 	@Override
 	public void publishModuleEvent(ModuleEvent type, Module module, Module origin) {
+		if (type == ModuleEvent.STARTING && slowdownEvents.get()) {
+			try {
+				Thread.sleep(6000);
+			} catch (InterruptedException e) {
+				// ignore
+				Thread.currentThread().interrupt();
+			}
+		}
 		moduleDatabase.addEvent(new DummyModuleEvent(module, type, module.getState()));
 	}
 
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 2bd9e9fb5..868e9b937 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
@@ -13,6 +13,7 @@ package org.eclipse.osgi.container;
 import java.util.Arrays;
 import java.util.EnumSet;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
 import org.eclipse.osgi.container.ModuleContainerAdaptor.ModuleEvent;
 import org.eclipse.osgi.internal.container.EquinoxReentrantLock;
 import org.eclipse.osgi.internal.debug.Debug;
@@ -159,7 +160,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 ThreadLocal<Boolean> inStartResolve = new ThreadLocal<Boolean>();
+	private final AtomicInteger inStartResolve = new AtomicInteger(0);
 	private volatile State state = State.INSTALLED;
 	private volatile int startlevel;
 	private volatile long lastModified;
@@ -401,18 +402,22 @@ public abstract class Module implements BundleReference, BundleStartLevel, Compa
 			if (State.ACTIVE.equals(getState()))
 				return;
 			if (getState().equals(State.INSTALLED)) {
-				// must unlock to avoid out of order locks when multiple unresolved
-				// bundles are started at the same time from different threads
-				unlockStateChange(ModuleEvent.STARTED);
-				lockedStarted = false;
 				ResolutionReport report;
+				inStartResolve.incrementAndGet();
 				try {
-					inStartResolve.set(Boolean.TRUE);
-					report = getRevisions().getContainer().resolve(Arrays.asList(this), true);
+					// must unlock to avoid out of order locks when multiple unresolved
+					// bundles are started at the same time from different threads
+					unlockStateChange(ModuleEvent.STARTED);
+					lockedStarted = false;
+					try {
+
+						report = getRevisions().getContainer().resolve(Arrays.asList(this), true);
+					} finally {
+						lockStateChange(ModuleEvent.STARTED);
+						lockedStarted = true;
+					}
 				} finally {
-					inStartResolve.set(Boolean.FALSE);
-					lockStateChange(ModuleEvent.STARTED);
-					lockedStarted = true;
+					inStartResolve.decrementAndGet();
 				}
 				// need to check valid again in case someone uninstalled the bundle
 				checkValid();
@@ -440,7 +445,6 @@ public abstract class Module implements BundleReference, BundleStartLevel, Compa
 				event = ModuleEvent.STOPPED;
 			}
 		} finally {
-			inStartResolve.set(Boolean.FALSE);
 			if (lockedStarted) {
 				unlockStateChange(ModuleEvent.STARTED);
 			}
@@ -689,10 +693,6 @@ public abstract class Module implements BundleReference, BundleStartLevel, Compa
 	}
 
 	final boolean inStartResolve() {
-		Boolean value = inStartResolve.get();
-		if (value == null) {
-			return false;
-		}
-		return value.booleanValue();
+		return inStartResolve.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 248d98d61..c7e02ab7c 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
@@ -623,9 +623,17 @@ public final class ModuleContainer implements DebugOptionsListener {
 				// lock.
 				for (Module module : modulesResolved) {
 					try {
+						// avoid grabbing the lock if the timestamp has changed
+						if (timestamp != moduleDatabase.getRevisionsTimestamp()) {
+							return false; // need to try again
+						}
 						module.lockStateChange(ModuleEvent.RESOLVED);
 						modulesLocked.add(module);
 					} catch (BundleException e) {
+						// before throwing an exception here, see if the timestamp has changed
+						if (timestamp != moduleDatabase.getRevisionsTimestamp()) {
+							return false; // need to try again
+						}
 						// TODO throw some appropriate exception
 						throw new IllegalStateException(Msg.ModuleContainer_StateLockError, e);
 					}
@@ -693,36 +701,39 @@ public final class ModuleContainer implements DebugOptionsListener {
 		Set<Module> triggerSet = restartTriggers ? new HashSet<Module>(triggers) : Collections.<Module> emptySet();
 		if (restartTriggers) {
 			for (Module module : triggers) {
-				try {
-					if (module.getId() != 0 && Module.RESOLVED_SET.contains(module.getState())) {
-						secureAction.start(module, StartOptions.TRANSIENT);
-					}
-				} catch (BundleException e) {
-					adaptor.publishContainerEvent(ContainerEvent.ERROR, module, e);
-				} catch (IllegalStateException e) {
-					// been uninstalled
-					continue;
+				if (module.getId() != 0 && Module.RESOLVED_SET.contains(module.getState())) {
+					start(module, StartOptions.TRANSIENT);
 				}
 			}
 		}
 		// 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)) {
-				continue;
-			}
-			try {
-				secureAction.start(module, StartOptions.TRANSIENT_IF_AUTO_START, StartOptions.TRANSIENT_RESUME);
-			} catch (BundleException e) {
-				adaptor.publishContainerEvent(ContainerEvent.ERROR, module, e);
-			} catch (IllegalStateException e) {
-				// been uninstalled
-				continue;
+			if (!module.inStartResolve() && module.getId() != 0 && !triggerSet.contains(module)) {
+				start(module, StartOptions.TRANSIENT_IF_AUTO_START, StartOptions.TRANSIENT_RESUME);
 			}
 		}
 		return true;
 	}
 
+	private void start(Module module, StartOptions... options) {
+		try {
+			secureAction.start(module, options);
+		} catch (BundleException e) {
+			if (e.getType() == BundleException.STATECHANGE_ERROR) {
+				if (Module.ACTIVE_SET.contains(module.getState())) {
+					// There is still a timing issue here;
+					// but at least try to detect that another thread is starting the module
+					return;
+				}
+			}
+			adaptor.publishContainerEvent(ContainerEvent.ERROR, module, e);
+		} catch (IllegalStateException e) {
+			// been uninstalled
+			return;
+		}
+	}
+
 	private List<DynamicModuleRequirement> getDynamicRequirements(String dynamicPkgName, ModuleRevision revision) {
 		// TODO Will likely need to optimize this
 		if ((revision.getTypes() & BundleRevision.TYPE_FRAGMENT) != 0) {