Further improvements to locking protocol. (FELIX-851)
git-svn-id: https://svn.apache.org/repos/asf/felix/trunk@749649 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/framework/src/main/java/org/apache/felix/framework/BundleImpl.java b/framework/src/main/java/org/apache/felix/framework/BundleImpl.java
index eaeb73f..5e0a2b1 100644
--- a/framework/src/main/java/org/apache/felix/framework/BundleImpl.java
+++ b/framework/src/main/java/org/apache/felix/framework/BundleImpl.java
@@ -1007,6 +1007,11 @@
return (m_lockCount == 0) || (m_lockThread == Thread.currentThread());
}
+ synchronized Thread getLockingThread()
+ {
+ return m_lockThread;
+ }
+
synchronized void lock()
{
if ((m_lockCount > 0) && (m_lockThread != Thread.currentThread()))
diff --git a/framework/src/main/java/org/apache/felix/framework/Felix.java b/framework/src/main/java/org/apache/felix/framework/Felix.java
index 4ce023f..056d63f 100644
--- a/framework/src/main/java/org/apache/felix/framework/Felix.java
+++ b/framework/src/main/java/org/apache/felix/framework/Felix.java
@@ -58,6 +58,8 @@
// Lock object used to determine if an individual bundle
// lock or the global lock can be acquired.
private final Object[] m_bundleLock = new Object[0];
+ // Keeps track of threads wanting to acquire the global lock.
+ private final List m_globalLockWaitersList = new ArrayList();
// The thread currently holding the global lock.
private Thread m_globalLockThread = null;
// How many times the global lock was acquired by the thread holding
@@ -1555,13 +1557,28 @@
try
{
+ // Revising the bundle creates a new module, which modifies
+ // the global state, so we need to acquire the global lock
+ // before revising.
+ boolean locked = acquireGlobalLock();
+ if (!locked)
+ {
+ throw new BundleException(
+ "Cannot acquire global lock to update the bundle.");
+ }
+ try
+ {
// REFACTOR - This adds the module to the resolver state, but should we do the
// security check first?
- bundle.revise(updateLocation, is);
+ bundle.revise(updateLocation, is);
+ }
+ finally
+ {
+ // Always release the global lock.
+ releaseGlobalLock();
+ }
- // Create a module for the new revision; the revision is
- // base zero, so subtract one from the revision count to
- // get the revision of the new update.
+ // Verify updated bundle.
try
{
Object sm = System.getSecurityManager();
@@ -1630,28 +1647,32 @@
fireBundleEvent(BundleEvent.UPDATED, bundle);
// Acquire global lock to check if we should auto-refresh.
- acquireGlobalLock();
-
- try
+ boolean locked = acquireGlobalLock();
+ // If we did not get the global lock, then do not try to
+ // auto-refresh.
+ if (locked)
{
- if (!bundle.isUsed())
+ try
{
- try
+ if (!bundle.isUsed())
{
- refreshPackages(new BundleImpl[] { bundle });
- }
- catch (Exception ex)
- {
- m_logger.log(
- Logger.LOG_ERROR,
- "Unable to immediately purge the bundle revisions.", ex);
+ try
+ {
+ refreshPackages(new BundleImpl[] { bundle });
+ }
+ catch (Exception ex)
+ {
+ m_logger.log(
+ Logger.LOG_ERROR,
+ "Unable to immediately purge the bundle revisions.", ex);
+ }
}
}
- }
- finally
- {
- // Always release the global lock.
- releaseGlobalLock();
+ finally
+ {
+ // Always release the global lock.
+ releaseGlobalLock();
+ }
}
}
@@ -1919,30 +1940,32 @@
fireBundleEvent(BundleEvent.UNINSTALLED, bundle);
// Acquire global lock to check if we should auto-refresh.
- acquireGlobalLock();
-
- try
+ boolean locked = acquireGlobalLock();
+ if (locked)
{
- // If the bundle is not used by anyone, then garbage
- // collect it now.
- if (!bundle.isUsed())
+ try
{
- try
+ // If the bundle is not used by anyone, then garbage
+ // collect it now.
+ if (!bundle.isUsed())
{
- refreshPackages(new BundleImpl[] { bundle });
- }
- catch (Exception ex)
- {
- m_logger.log(
- Logger.LOG_ERROR,
- "Unable to immediately garbage collect the bundle.", ex);
+ try
+ {
+ refreshPackages(new BundleImpl[] { bundle });
+ }
+ catch (Exception ex)
+ {
+ m_logger.log(
+ Logger.LOG_ERROR,
+ "Unable to immediately garbage collect the bundle.", ex);
+ }
}
}
- }
- finally
- {
- // Always release the global lock.
- releaseGlobalLock();
+ finally
+ {
+ // Always release the global lock.
+ releaseGlobalLock();
+ }
}
}
@@ -2055,8 +2078,23 @@
{
BundleArchive archive = m_cache.getArchive(id);
- // Create bundle.
- bundle = new BundleImpl(this, archive);
+ // Acquire the global lock to create the bundle,
+ // since this impacts the global state.
+ boolean locked = acquireGlobalLock();
+ if (!locked)
+ {
+ throw new BundleException(
+ "Unable to acquire the global lock to install the bundle.");
+ }
+ try
+ {
+ bundle = new BundleImpl(this, archive);
+ }
+ finally
+ {
+ // Always release the global lock.
+ releaseGlobalLock();
+ }
verifyExecutionEnvironment(bundle);
@@ -2917,38 +2955,46 @@
boolean resolveBundles(Bundle[] targets)
{
// Acquire global lock.
- acquireGlobalLock();
-
- // Determine set of bundles to be resolved, which is either the
- // specified bundles or all bundles if null.
- if (targets == null)
+ boolean locked = acquireGlobalLock();
+ if (!locked)
{
- List list = new ArrayList();
+ m_logger.log(
+ Logger.LOG_WARNING,
+ "Unable to acquire global lock to perform resolve.",
+ null);
+ return false;
+ }
- // Add all unresolved bundles to the list.
- synchronized (m_installedBundleLock_Priority2)
+ try
+ {
+ // Determine set of bundles to be resolved, which is either the
+ // specified bundles or all bundles if null.
+ if (targets == null)
{
- Iterator iter = m_installedBundleMap.values().iterator();
- while (iter.hasNext())
+ List list = new ArrayList();
+
+ // Add all unresolved bundles to the list.
+ synchronized (m_installedBundleLock_Priority2)
{
- BundleImpl bundle = (BundleImpl) iter.next();
- if (bundle.getState() == Bundle.INSTALLED)
+ Iterator iter = m_installedBundleMap.values().iterator();
+ while (iter.hasNext())
{
- list.add(bundle);
+ BundleImpl bundle = (BundleImpl) iter.next();
+ if (bundle.getState() == Bundle.INSTALLED)
+ {
+ list.add(bundle);
+ }
}
}
+
+ // Create an array.
+ if (list.size() > 0)
+ {
+ targets = (Bundle[]) list.toArray(new BundleImpl[list.size()]);
+ }
}
- // Create an array.
- if (list.size() > 0)
- {
- targets = (Bundle[]) list.toArray(new BundleImpl[list.size()]);
- }
- }
-
- // Now resolve each target bundle.
- try
- {
+ // Now resolve each target bundle.
boolean result = true;
// If there are targets, then resolve each one.
@@ -3002,7 +3048,17 @@
void refreshPackages(Bundle[] targets)
{
// Acquire global lock.
- acquireGlobalLock();
+ boolean locked = acquireGlobalLock();
+ if (!locked)
+ {
+ // If the thread calling holds bundle locks, then we might not
+ // be able to get the global lock. However, in practice this
+ // should not happen since the calls to this method have either
+ // already acquired the global lock or it is PackageAdmin which
+ // doesn't hold bundle locks.
+ throw new IllegalStateException(
+ "Unable to acquire global lock for refresh.");
+ }
// Determine set of bundles to refresh, which is all transitive
// dependencies of specified set or all transitive dependencies
@@ -3513,7 +3569,12 @@
if (!rootModule.isResolved())
{
// Acquire global lock.
- acquireGlobalLock();
+ boolean locked = acquireGlobalLock();
+ if (!locked)
+ {
+ throw new ResolveException(
+ "Unable to acquire global lock for resolve.", rootModule, null);
+ }
try
{
@@ -3596,7 +3657,12 @@
if (importer.isResolved())
{
// Acquire global lock.
- acquireGlobalLock();
+ boolean locked = acquireGlobalLock();
+ if (!locked)
+ {
+ throw new ResolveException(
+ "Unable to acquire global lock for resolve.", importer, null);
+ }
try
{
@@ -4138,6 +4204,16 @@
{
throw new IllegalStateException("Bundle in unexpected state.");
}
+ // If the calling thread already owns the global lock, then make
+ // sure no other thread is trying to promote a bundle lock to a
+ // global lock. If so, interrupt the other thread to avoid deadlock.
+ else if (m_globalLockThread == Thread.currentThread()
+ && (bundle.getLockingThread() != null)
+ && m_globalLockWaitersList.contains(bundle.getLockingThread()))
+ {
+ bundle.getLockingThread().interrupt();
+ }
+
try
{
m_bundleLock.wait();
@@ -4173,35 +4249,73 @@
{
// Unlock the bundle.
bundle.unlock();
- m_bundleLock.notifyAll();
+ // If the thread no longer holds the bundle lock,
+ // then remove it from the held lock map.
+ if (bundle.getLockingThread() == null)
+ {
+ m_bundleLock.notifyAll();
+ }
}
}
/**
- * Acquires the global lock, which is necessary for multi-bundle operations
- * like refreshing and resolving.
+ * Promotes a bundle lock to the global lock. This is called by a thread
+ * wanting the global lock, but already holding a bundle lock (currently
+ * only when updating a bundle). Since it is possible to deadlock when
+ * trying to acquire the global lock while holding a bundle lock, this
+ * method may fail if a potential deadlock is detected.
+ * @param bundle The bundle already locked by the calling thread.
+ * @return <tt>true</tt> if the global lock was successfully acquired,
+ * <tt>false</tt> otherwise.
**/
- private void acquireGlobalLock()
+ private boolean acquireGlobalLock()
{
synchronized (m_bundleLock)
{
- // Wait as long as some other thread holds the global lock.
- while ((m_globalLockThread != null)
+ // Wait as long as some other thread holds the global lock
+ // and the current thread is not interrupted.
+ boolean interrupted = false;
+ while (!interrupted
+ && (m_globalLockThread != null)
&& (m_globalLockThread != Thread.currentThread()))
{
+ // Add calling thread to global lock waiters list.
+ m_globalLockWaitersList.add(Thread.currentThread());
+ // We need to wake up all waiting threads so we can
+ // recheck for potential deadlock in acquireBundleLock()
+ // if this thread was holding a bundle lock and is now
+ // trying to promote it to a global lock.
+ m_bundleLock.notifyAll();
+ // Now wait for the global lock.
try
{
m_bundleLock.wait();
}
catch (InterruptedException ex)
{
- // Ignore
+ interrupted = true;
}
+ // At this point we are either interrupted or will get the
+ // global lock, so remove the thread from the waiters list.
+ m_globalLockWaitersList.remove(Thread.currentThread());
}
- // Increment the current thread's global lock count.
- m_globalLockCount++;
- m_globalLockThread = Thread.currentThread();
+ // Check to see if we were interrupted, which means someone
+ // with the global lock wants our bundle lock, so we should
+ // fail gracefully.
+ if (!interrupted)
+ {
+ // Increment the current thread's global lock count.
+ m_globalLockCount++;
+ m_globalLockThread = Thread.currentThread();
+ }
+
+ // Note: If the thread was interrupted, there is no reason to notify
+ // anyone, since the thread was likely interrupted to force it to give
+ // up a bundle lock it is holding. When it does give up the bundle
+ // lock, it will do a notifyAll() in there.
+
+ return !interrupted;
}
}