Bug 496034 - Obtain readLock on ModuleDatabase before classLoaderMonitor

To prevent potential out of order locks between the ModuleDatabase 
read/write lock and the classLoaderMonitor lock we obtain the readLock
on the ModuleDatabase before the classLoaderMonitor.  This is because
during the refresh operation the removed wirings will close the
BundleLoader while holding the writeLock, this eventually nests a lock
with the classLoaderMonitor monitor lock.  Since we call foreign hook
code while holding the classLoaderMonitor monitor lock we should be
consistent in the order locks are obtained to prevent potential
deadlocks by the hooks.

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

1 files changed, 37 insertions, 13 deletions

diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/loader/BundleLoader.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/loader/BundleLoader.java
index 2e7f0e319..4e0f698dd 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/loader/BundleLoader.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/loader/BundleLoader.java
@@ -96,7 +96,7 @@ public class BundleLoader extends ModuleLoader {
 
 	private Object classLoaderMonitor = new Object();
 	/* @GuardedBy("classLoaderMonitor") */
-	private ModuleClassLoader classloader;
+	private volatile ModuleClassLoader classloader;
 	private final ClassLoader parent;
 	private final AtomicBoolean triggerClassLoaded = new AtomicBoolean(false);
 
@@ -220,8 +220,14 @@ public class BundleLoader extends ModuleLoader {
 	}
 
 	public ModuleClassLoader getModuleClassLoader() {
-		synchronized (classLoaderMonitor) {
-			if (classloader == null) {
+		if (classloader != null) {
+			return classloader;
+		}
+		// Need to obtain readlock on module database to prevent out of order locks that can happen
+		// when a refresh operation (requiring write lock) closes a BundleLoader
+		container.getStorage().getModuleDatabase().readLock();
+		try {
+			synchronized (classLoaderMonitor) {
 				final List<ClassLoaderHook> hooks = container.getConfiguration().getHookRegistry().getClassLoaderHooks();
 				final Generation generation = (Generation) wiring.getRevision().getRevisionInfo();
 				if (System.getSecurityManager() == null) {
@@ -238,17 +244,26 @@ public class BundleLoader extends ModuleLoader {
 				for (ClassLoaderHook hook : hooks) {
 					hook.classLoaderCreated(classloader);
 				}
+				return classloader;
 			}
-			return classloader;
+		} finally {
+			container.getStorage().getModuleDatabase().readUnlock();
 		}
 	}
 
 	@Override
 	protected void loadFragments(Collection<ModuleRevision> fragments) {
-		synchronized (classLoaderMonitor) {
-			addFragmentExports(wiring.getModuleCapabilities(PackageNamespace.PACKAGE_NAMESPACE));
-			loadClassLoaderFragments(fragments);
-			clearManifestLocalizationCache();
+		// Need to obtain readlock on module database to prevent out of order locks that can happen
+		// when a refresh operation (requiring write lock) closes a BundleLoader
+		container.getStorage().getModuleDatabase().readLock();
+		try {
+			synchronized (classLoaderMonitor) {
+				addFragmentExports(wiring.getModuleCapabilities(PackageNamespace.PACKAGE_NAMESPACE));
+				loadClassLoaderFragments(fragments);
+				clearManifestLocalizationCache();
+			}
+		} finally {
+			container.getStorage().getModuleDatabase().readUnlock();
 		}
 	}
 
@@ -289,10 +304,17 @@ public class BundleLoader extends ModuleLoader {
 				policy.close(context);
 			}
 		}
-		synchronized (classLoaderMonitor) {
-			if (classloader != null) {
-				classloader.close();
+		// Need to obtain readlock on module database to prevent out of order locks that can happen
+		// when a refresh operation (requiring write lock) closes a BundleLoader
+		container.getStorage().getModuleDatabase().readLock();
+		try {
+			synchronized (classLoaderMonitor) {
+				if (classloader != null) {
+					classloader.close();
+				}
 			}
+		} finally {
+			container.getStorage().getModuleDatabase().readUnlock();
 		}
 	}
 
@@ -367,7 +389,7 @@ public class BundleLoader extends ModuleLoader {
 		String pkgName = getPackageName(name);
 		boolean bootDelegation = false;
 		// follow the OSGi delegation model
-		if (checkParent && parent != null && container.isBootDelegationPackage(pkgName))
+		if (checkParent && parent != null && container.isBootDelegationPackage(pkgName)) {
 			// 2) if part of the bootdelegation list then delegate to parent and continue of failure
 			try {
 				return parent.loadClass(name);
@@ -375,6 +397,7 @@ public class BundleLoader extends ModuleLoader {
 				// we want to continue
 				bootDelegation = true;
 			}
+		}
 		Class<?> result = null;
 		try {
 			result = (Class<?>) searchHooks(name, PRE_CLASS);
@@ -438,13 +461,14 @@ public class BundleLoader extends ModuleLoader {
 			return result;
 		// hack to support backwards compatibility for bootdelegation
 		// or last resort; do class context trick to work around VM bugs
-		if (parent != null && !bootDelegation && ((checkParent && container.getConfiguration().compatibilityBootDelegation) || isRequestFromVM()))
+		if (parent != null && !bootDelegation && ((checkParent && container.getConfiguration().compatibilityBootDelegation) || isRequestFromVM())) {
 			// we don't need to continue if a CNFE is thrown here.
 			try {
 				return parent.loadClass(name);
 			} catch (ClassNotFoundException e) {
 				// we want to generate our own exception below
 			}
+		}
 		throw new ClassNotFoundException(name + " cannot be found by " + this); //$NON-NLS-1$
 	}