Bug 541755 - Holding resolution lock while firing events cause deadlock
Change-Id: I81c456b8f8691497c83aebb4b87c3e33171b6574
Signed-off-by: Thomas Watson <tjwatson@us.ibm.com>
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 fb9c528..57ef9d4 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
@@ -49,6 +49,8 @@
import java.util.concurrent.ThreadFactory;
import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
import java.util.jar.Attributes;
import java.util.jar.Manifest;
import org.eclipse.osgi.container.Module;
@@ -2864,7 +2866,7 @@
}
adaptor.setSlowdownEvents(true);
final ConcurrentLinkedQueue<BundleException> startErrors = new ConcurrentLinkedQueue<BundleException>();
- final ExecutorService executor = Executors.newFixedThreadPool(10);
+ final ExecutorService executor = Executors.newFixedThreadPool(5);
try {
for (final Module module : modules) {
@@ -2894,6 +2896,107 @@
}
}
+ class RecurseResolverHook implements ResolverHook {
+ volatile ModuleContainer container;
+ volatile Module dynamicImport;
+ final AtomicInteger id = new AtomicInteger();
+ List<IllegalStateException> expectedErrors = Collections.synchronizedList(new ArrayList<IllegalStateException>());
+
+ @Override
+ public void filterResolvable(Collection<BundleRevision> candidates) {
+ ModuleContainer current = container;
+ if (current != null) {
+ int nextId = id.incrementAndGet();
+ if (nextId >= 2) {
+ // Don't do this again
+ return;
+ }
+ Map<String, String> manifest = new HashMap<String, String>();
+ manifest.put(Constants.BUNDLE_MANIFESTVERSION, "2");
+ manifest.put(Constants.BUNDLE_SYMBOLICNAME, "module.recurse." + nextId);
+ try {
+ Module m = installDummyModule(manifest, manifest.get(Constants.BUNDLE_SYMBOLICNAME), current);
+ ResolutionReport report = current.resolve(Collections.singleton(m), false);
+ report.getResolutionException();
+ } catch (IllegalStateException e) {
+ expectedErrors.add(e);
+ } catch (BundleException e) {
+ // TODO Auto-generated catch block
+ e.printStackTrace();
+ }
+
+ Module curDynamicImport = dynamicImport;
+ if (curDynamicImport != null) {
+ try {
+ current.resolveDynamic("org.osgi.framework", curDynamicImport.getCurrentRevision());
+ } catch (IllegalStateException e) {
+ expectedErrors.add(e);
+ }
+ }
+ }
+ }
+
+ @Override
+ public void filterSingletonCollisions(BundleCapability singleton, Collection<BundleCapability> collisionCandidates) {
+ // nothing
+ }
+
+ @Override
+ public void filterMatches(BundleRequirement requirement, Collection<BundleCapability> candidates) {
+ // nothing
+ }
+
+ @Override
+ public void end() {
+ // nothing
+ }
+
+ List<IllegalStateException> getExpectedErrors() {
+ return new ArrayList<>(expectedErrors);
+ }
+ }
+
+ @Test
+ public void testRecurseResolutionPermits() throws BundleException, IOException {
+ RecurseResolverHook resolverHook = new RecurseResolverHook();
+ DummyContainerAdaptor adaptor = createDummyAdaptor(resolverHook);
+ final 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 bundle to do dynamic resolution from
+ Map<String, String> manifest = new HashMap<String, String>();
+ manifest.put(Constants.BUNDLE_MANIFESTVERSION, "2");
+ manifest.put(Constants.BUNDLE_SYMBOLICNAME, "dynamicImport");
+ manifest.put(Constants.DYNAMICIMPORT_PACKAGE, "*");
+ final Module dynamicImport = installDummyModule(manifest, manifest.get(Constants.BUNDLE_SYMBOLICNAME), container);
+ dynamicImport.start();
+ resolverHook.dynamicImport = dynamicImport;
+ resolverHook.container = container;
+
+ final AtomicReference<ModuleWire> dynamicWire = new AtomicReference<>();
+ Runnable runForEvents = new Runnable() {
+ @Override
+ public void run() {
+ dynamicWire.set(container.resolveDynamic("org.osgi.framework", dynamicImport.getCurrentRevision()));
+ }
+ };
+ adaptor.setRunForEvents(runForEvents);
+ // install a bundle to resolve
+ manifest.clear();
+ manifest.put(Constants.BUNDLE_MANIFESTVERSION, "2");
+ manifest.put(Constants.BUNDLE_SYMBOLICNAME, "initial");
+ Module m = installDummyModule(manifest, manifest.get(Constants.BUNDLE_SYMBOLICNAME), container);
+ m.start();
+
+ assertNotNull("No Dynamic Wire", dynamicWire.get());
+ assertEquals("Wrong number of exected errors.", 2, resolverHook.getExpectedErrors().size());
+ }
+
@Test
public void testSystemBundleFragmentsPackageImport() throws BundleException, IOException {
// install the system.bundle
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 077b5ba..5b14a54 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
@@ -32,6 +32,7 @@
public class DummyContainerAdaptor extends ModuleContainerAdaptor {
private AtomicBoolean slowdownEvents = new AtomicBoolean(false);
+ private Runnable runForEvents = null;
private final ModuleCollisionHook collisionHook;
private final Map<String, String> configuration;
private final DummyModuleDatabase moduleDatabase;
@@ -111,9 +112,17 @@
Thread.currentThread().interrupt();
}
}
+ Runnable current = runForEvents;
+ if (current != null) {
+ current.run();
+ }
moduleDatabase.addEvent(new DummyModuleEvent(module, type, module.getState()));
}
+ public void setRunForEvents(Runnable runForEvents) {
+ this.runForEvents = runForEvents;
+ }
+
@Override
public DebugOptions getDebugOptions() {
return this.debugOptions;
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 73bfac5..1372e0d 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
@@ -28,12 +28,15 @@
import java.util.Set;
import java.util.concurrent.Semaphore;
import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReference;
import java.util.concurrent.locks.ReentrantLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
import org.eclipse.osgi.container.Module.StartOptions;
import org.eclipse.osgi.container.Module.State;
import org.eclipse.osgi.container.Module.StopOptions;
+import org.eclipse.osgi.container.ModuleContainer.ResolutionLock.Permits;
import org.eclipse.osgi.container.ModuleContainerAdaptor.ContainerEvent;
import org.eclipse.osgi.container.ModuleContainerAdaptor.ModuleEvent;
import org.eclipse.osgi.container.ModuleDatabase.Sort;
@@ -485,10 +488,10 @@
return new ModuleResolutionReport(null, Collections.<Resource, List<Entry>> emptyMap(), new ResolutionException("Unable to resolve while shutting down the framework.")); //$NON-NLS-1$
}
ResolutionReport report = null;
- try (ResolutionLock resolutionLock = new ResolutionLock(1)) {
+ try (ResolutionLock.Permits resolutionPermits = _resolutionLock.acquire(1)) {
do {
try {
- report = resolveAndApply(triggers, triggersMandatory, restartTriggers);
+ report = resolveAndApply(triggers, triggersMandatory, restartTriggers, resolutionPermits);
} catch (RuntimeException e) {
if (e.getCause() instanceof BundleException) {
BundleException be = (BundleException) e.getCause();
@@ -505,7 +508,7 @@
return report;
}
- private ResolutionReport resolveAndApply(Collection<Module> triggers, boolean triggersMandatory, boolean restartTriggers) {
+ private ResolutionReport resolveAndApply(Collection<Module> triggers, boolean triggersMandatory, boolean restartTriggers, ResolutionLock.Permits resolutionPermits) {
if (triggers == null)
triggers = new ArrayList<>(0);
Collection<ModuleRevision> triggerRevisions = new ArrayList<>(triggers.size());
@@ -545,7 +548,7 @@
modulesResolved.add(deltaRevision.getRevisions().getModule());
}
- return applyDelta(deltaWiring, modulesResolved, triggers, timestamp, restartTriggers) ? report : null;
+ return applyDelta(deltaWiring, modulesResolved, triggers, timestamp, restartTriggers, resolutionPermits) ? report : null;
}
/**
@@ -560,7 +563,7 @@
Map<ModuleRevision, ModuleWiring> deltaWiring;
Collection<Module> modulesResolved;
long timestamp;
- try (ResolutionLock resolutionLock = new ResolutionLock(MAX_RESOLUTION_PERMITS)) {
+ try (Permits resolutionPermits = _resolutionLock.acquire(ResolutionLock.MAX_RESOLUTION_PERMITS)) {
do {
result = null;
Map<ModuleRevision, ModuleWiring> wiringClone = null;
@@ -644,7 +647,7 @@
// Save the result
ModuleWiring wiring = deltaWiring.get(revision);
result = findExistingDynamicWire(wiring, dynamicPkgName);
- } while (!applyDelta(deltaWiring, modulesResolved, Collections.<Module> emptyList(), timestamp, false));
+ } while (!applyDelta(deltaWiring, modulesResolved, Collections.<Module> emptyList(), timestamp, false, resolutionPermits));
} catch (ResolutionLockException e) {
return null;
}
@@ -681,8 +684,7 @@
// between taking the snapshot and successfully applying the
// results. Instead of resorting to single threaded operations
// we choose to limit the number of concurrent resolves
- final static int MAX_RESOLUTION_PERMITS = 10;
- final Semaphore _resolutionLock = new Semaphore(MAX_RESOLUTION_PERMITS);
+ final ResolutionLock _resolutionLock = new ResolutionLock();
final ReentrantLock _bundleStateLock = new ReentrantLock();
static class ResolutionLockException extends Exception {
@@ -697,32 +699,61 @@
}
}
- class ResolutionLock implements Closeable {
- private final int permits;
+ /**
+ * A resolution hook allows for a max of 10 threads to do resolution operations
+ * at the same time. The implementation uses a semaphore to grant the max number
+ * of permits (threads) but a reentrant read lock is also used to detect reentry.
+ * If a thread reenters then no extra permits are required by the thread.
+ * This lock returns a Permits object that implements closeable for use in
+ * try->with. If permits is closed multiple times then the additional close
+ * operations are a no-op.
+ */
+ static class ResolutionLock {
+ final static int MAX_RESOLUTION_PERMITS = 10;
+ final Semaphore permitPool = new Semaphore(MAX_RESOLUTION_PERMITS);
+ final ReentrantReadWriteLock reenterLock = new ReentrantReadWriteLock();
- ResolutionLock(int permits) throws ResolutionLockException {
- this.permits = permits;
- boolean previousInterruption = Thread.interrupted();
- try {
- if (!_resolutionLock.tryAcquire(permits, 30, TimeUnit.SECONDS)) {
- throw new ResolutionLockException();
+ class Permits implements Closeable {
+ private final int aquiredPermits;
+ private final AtomicBoolean closed = new AtomicBoolean();
+
+ Permits(int requestedPermits) throws ResolutionLockException {
+ if (reenterLock.getReadHoldCount() > 0) {
+ // thread is already holding permits; don't request more
+ requestedPermits = 0;
}
- } catch (InterruptedException e) {
- throw new ResolutionLockException(e);
- } finally {
- if (previousInterruption) {
- Thread.currentThread().interrupt();
+ this.aquiredPermits = requestedPermits;
+ boolean previousInterruption = Thread.interrupted();
+ try {
+ if (!permitPool.tryAcquire(requestedPermits, 30, TimeUnit.SECONDS)) {
+ throw new ResolutionLockException();
+ }
+ } catch (InterruptedException e) {
+ throw new ResolutionLockException(e);
+ } finally {
+ if (previousInterruption) {
+ Thread.currentThread().interrupt();
+ }
+ }
+ // mark this thread as holding permits
+ reenterLock.readLock().lock();
+ }
+
+ @Override
+ public void close() {
+ if (closed.compareAndSet(false, true)) {
+ permitPool.release(aquiredPermits);
+ reenterLock.readLock().unlock();
}
}
}
- @Override
- public void close() {
- _resolutionLock.release(permits);
+ Permits acquire(int requestedPermits) throws ResolutionLockException {
+ return new Permits(requestedPermits);
}
}
- private boolean applyDelta(Map<ModuleRevision, ModuleWiring> deltaWiring, Collection<Module> modulesResolved, Collection<Module> triggers, long timestamp, boolean restartTriggers) {
+ private boolean applyDelta(Map<ModuleRevision, ModuleWiring> deltaWiring, Collection<Module> modulesResolved, Collection<Module> triggers, long timestamp, boolean restartTriggers, ResolutionLock.Permits resolutionPermits) {
List<Module> modulesLocked = new ArrayList<>(modulesResolved.size());
// now attempt to apply the delta
try {
@@ -809,6 +840,9 @@
}
}
+ // release resolution permits before firing events
+ resolutionPermits.close();
+
for (Module module : modulesLocked) {
adaptor.publishModuleEvent(ModuleEvent.RESOLVED, module, module);
}
