diff options
author | Thomas Watson | 2018-11-30 21:06:35 +0000 |
---|---|---|
committer | Thomas Watson | 2018-11-30 21:06:35 +0000 |
commit | 98cb609adbf0b986d30720f8b62d744e5c39e3e1 (patch) | |
tree | 993190a8ff4e603670a4085b098af22cc4b49aa9 | |
parent | 2c1856c0aa73d33f15b0e6ade7c85728338b03bd (diff) | |
download | rt.equinox.framework-R4_10_maintenance.tar.gz rt.equinox.framework-R4_10_maintenance.tar.xz rt.equinox.framework-R4_10_maintenance.zip |
Bug 541755 - Holding resolution lock while firing events cause deadlockY20181205-2200Y20181204-0315S4_10_0_RC2R4_10I20181206-0815I20181206-0320I20181206-0225I20181206-0030I20181205-1800I20181205-0600I20181204-1800I20181204-0600I20181203-1800I20181203-0600I20181202-1800I20181202-0600I20181201-1800I20181201-0600I20181130-1800R4_10_maintenance
Change-Id: I81c456b8f8691497c83aebb4b87c3e33171b6574
Signed-off-by: Thomas Watson <tjwatson@us.ibm.com>
3 files changed, 173 insertions, 27 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 fb9c5289b..57ef9d4ed 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.SynchronousQueue; 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 @@ public class TestModuleContainer extends AbstractTest { } 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 @@ public class TestModuleContainer extends AbstractTest { } } + 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 077b5ba74..5b14a5469 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 @@ import org.osgi.framework.hooks.resolver.ResolverHookFactory; 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 @@ public class DummyContainerAdaptor extends ModuleContainerAdaptor { 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 73bfac5bb..1372e0d55 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.Map; 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 @@ public final class ModuleContainer implements DebugOptionsListener { 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 @@ public final class ModuleContainer implements DebugOptionsListener { 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 @@ public final class ModuleContainer implements DebugOptionsListener { 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 @@ public final class ModuleContainer implements DebugOptionsListener { 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 @@ public final class ModuleContainer implements DebugOptionsListener { // 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 @@ public final class ModuleContainer implements DebugOptionsListener { // 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 @@ public final class ModuleContainer implements DebugOptionsListener { } } - class ResolutionLock implements Closeable { - private final int permits; - - ResolutionLock(int permits) throws ResolutionLockException { - this.permits = permits; - boolean previousInterruption = Thread.interrupted(); - try { - if (!_resolutionLock.tryAcquire(permits, 30, TimeUnit.SECONDS)) { - throw new ResolutionLockException(); + /** + * 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(); + + 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 @@ public final class ModuleContainer implements DebugOptionsListener { } } + // release resolution permits before firing events + resolutionPermits.close(); + for (Module module : modulesLocked) { adaptor.publishModuleEvent(ModuleEvent.RESOLVED, module, module); } |