From a74bbe6ad8d381c7b28f46cdd2c5ff52c6bc247e Mon Sep 17 00:00:00 2001 From: Thomas Watson Date: Fri, 15 Nov 2024 13:52:22 -0600 Subject: [PATCH] Get loader-classname lock for calls to findLoadedClass This is related to Open J9 issue https://github.com/eclipse-openj9/openj9/issues/20444 There is a concern that the initial call to findLoadedClass by Equinox is not protected by the same loader-classname lock as the later calls to findLoadedClass/defineClass are. There are concerns that the unprotected findLoadedClass call could catch the JVM in an invalid state if there is another thread in the middle of defining the class. This change replaces the clunky way of doing loader-classname locks with a more efficient strategy. This way we can use this lock around the initial call to findLoadedClass without too much concern over the performance impact. This change also removes some of the old code that still supported Java 6. It is always assumed we now can register the class loaders as parallel capable. --- .../internal/container/KeyBasedLockStore.java | 71 +++++++++++ .../internal/loader/EquinoxClassLoader.java | 15 +-- .../internal/loader/ModuleClassLoader.java | 110 ++++++------------ 3 files changed, 111 insertions(+), 85 deletions(-) create mode 100644 bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/container/KeyBasedLockStore.java diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/container/KeyBasedLockStore.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/container/KeyBasedLockStore.java new file mode 100644 index 00000000000..e4b5e9d3828 --- /dev/null +++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/container/KeyBasedLockStore.java @@ -0,0 +1,71 @@ +/******************************************************************************* + * Copyright (c) 2022, 2024 IBM Corporation and others. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + * + * Contributors: + * IBM Corporation - initial API and implementation + *******************************************************************************/ +package org.eclipse.osgi.internal.container; + +import java.lang.ref.ReferenceQueue; +import java.lang.ref.WeakReference; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Function; + +public final class KeyBasedLockStore { + + final ReferenceQueue refQueue = new ReferenceQueue<>(); + private final ConcurrentHashMap lockMap = new ConcurrentHashMap<>(); + private final Function lockCreator; + + public KeyBasedLockStore(Function lockCreator) { + this.lockCreator = lockCreator; + } + + private final class LockWeakRef extends WeakReference { + final Key key; + + public LockWeakRef(Lock referent, Key keyValue) { + super(referent, refQueue); + key = keyValue; + } + } + + public final Lock getLock(Key key) { + poll(); + LockWeakRef lockRef = lockMap.get(key); + Lock lock = lockRef != null ? lockRef.get() : null; + if (lock != null) { + return lock; + } + + lock = lockCreator.apply(key); + + while (true) { + LockWeakRef retVal = lockMap.putIfAbsent(key, new LockWeakRef(lock, key)); + if (retVal == null) { + return lock; + } + + Lock retLock = retVal.get(); + if (retLock != null) { + return retLock; + } + lockMap.remove(key, retVal); + } + } + + @SuppressWarnings("unchecked") + private final void poll() { + LockWeakRef lockRef; + while ((lockRef = (LockWeakRef) refQueue.poll()) != null) { + lockMap.remove(lockRef.key, lockRef); + } + } +} diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/loader/EquinoxClassLoader.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/loader/EquinoxClassLoader.java index c834e453413..b7defd1509a 100644 --- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/loader/EquinoxClassLoader.java +++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/loader/EquinoxClassLoader.java @@ -20,15 +20,12 @@ import org.eclipse.osgi.storage.BundleInfo.Generation; public class EquinoxClassLoader extends ModuleClassLoader { - protected static final boolean EQUINOX_REGISTERED_AS_PARALLEL; static { - boolean registered; try { - registered = ClassLoader.registerAsParallelCapable(); + ClassLoader.registerAsParallelCapable(); } catch (Throwable t) { - registered = false; + // ignore all exceptions; substrate native image fails here } - EQUINOX_REGISTERED_AS_PARALLEL = registered; } private final EquinoxConfiguration configuration; private final Debug debug; @@ -37,7 +34,6 @@ public class EquinoxClassLoader extends ModuleClassLoader { // TODO Note that PDE has internal dependency on this field type/name (bug // 267238) private final ClasspathManager manager; - private final boolean isRegisteredAsParallel; /** * Constructs a new DefaultClassLoader. @@ -55,8 +51,6 @@ public EquinoxClassLoader(ClassLoader parent, EquinoxConfiguration configuration this.delegate = delegate; this.generation = generation; this.manager = new ClasspathManager(generation, this); - this.isRegisteredAsParallel = (ModuleClassLoader.REGISTERED_AS_PARALLEL && EQUINOX_REGISTERED_AS_PARALLEL) - || this.configuration.PARALLEL_CAPABLE; } @Override @@ -69,11 +63,6 @@ public final ClasspathManager getClasspathManager() { return manager; } - @Override - public final boolean isRegisteredAsParallel() { - return isRegisteredAsParallel; - } - @Override public final BundleLoader getBundleLoader() { return delegate; diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/loader/ModuleClassLoader.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/loader/ModuleClassLoader.java index c586190d334..0d6e7e7ba18 100644 --- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/loader/ModuleClassLoader.java +++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/loader/ModuleClassLoader.java @@ -25,10 +25,10 @@ import java.security.cert.Certificate; import java.util.Collection; import java.util.Enumeration; -import java.util.HashMap; import java.util.List; -import java.util.Map; +import java.util.function.Function; import org.eclipse.osgi.container.ModuleRevision; +import org.eclipse.osgi.internal.container.KeyBasedLockStore; import org.eclipse.osgi.internal.debug.Debug; import org.eclipse.osgi.internal.framework.EquinoxConfiguration; import org.eclipse.osgi.internal.loader.classpath.ClasspathEntry; @@ -61,15 +61,12 @@ public Bundle getBundle() { * ProtectionDomains when security is disabled */ protected static final PermissionCollection ALLPERMISSIONS; - protected static final boolean REGISTERED_AS_PARALLEL; static { - boolean registered; try { - registered = ClassLoader.registerAsParallelCapable(); + ClassLoader.registerAsParallelCapable(); } catch (Throwable t) { - registered = false; + // ignore all exceptions; substrate native image fails here } - REGISTERED_AS_PARALLEL = registered; } static { @@ -100,7 +97,26 @@ public DefineClassResult(Class clazz, boolean defined) { } } - private final Map classNameLocks = new HashMap<>(5); + private static final class ClassNameLock { + static final Function SUPPLIER = new Function() { + public ClassNameLock apply(String className) { + return new ClassNameLock(className); + } + }; + final String name; + + ClassNameLock(String name) { + this.name = name; + } + + @Override + public String toString() { + return "ClassNameLock: " + name; //$NON-NLS-1$ + } + } + + private final KeyBasedLockStore classNameLocks = new KeyBasedLockStore<>( + ClassNameLock.SUPPLIER); private final Object pkgLock = new Object(); /** @@ -149,12 +165,15 @@ public ModuleClassLoader(ClassLoader parent) { /** * Returns true if this class loader implementation has been registered with the - * JVM as a parallel class loader. This requires Java 7 or later. + * JVM as a parallel class loader. This requires Java 7 or later. This always + * returns true now that Java 8 is required. * * @return true if this class loader implementation has been registered with the * JVM as a parallel class loader; otherwise false is returned. */ - public abstract boolean isRegisteredAsParallel(); + public boolean isRegisteredAsParallel() { + return true; + } /** * Loads a class for the bundle. First delegate.findClass(name) is called. The @@ -284,38 +303,18 @@ public DefineClassResult defineClass(String name, byte[] classbytes, ClasspathEn // See ClasspathManager.findLocalClass(String) boolean defined = false; Class result = null; - if (isRegisteredAsParallel()) { - // lock by class name in this case - boolean initialLock = lockClassName(name); - try { - result = findLoadedClass(name); - if (result == null) { - result = defineClass(name, classbytes, 0, classbytes.length, classpathEntry.getDomain()); - defined = true; - } - } finally { - if (initialLock) { - unlockClassName(name); - } - } - } else { - // lock by class loader instance in this case - synchronized (this) { - result = findLoadedClass(name); - if (result == null) { - result = defineClass(name, classbytes, 0, classbytes.length, classpathEntry.getDomain()); - defined = true; - } + synchronized (getClassLoadingLock(name)) { + result = findLoadedClass(name); + if (result == null) { + result = defineClass(name, classbytes, 0, classbytes.length, classpathEntry.getDomain()); + defined = true; } } return new DefineClassResult(result, defined); } public Class publicFindLoaded(String classname) { - if (isRegisteredAsParallel()) { - return findLoadedClass(classname); - } - synchronized (this) { + synchronized (getClassLoadingLock(classname)) { return findLoadedClass(classname); } } @@ -427,42 +426,9 @@ public void loadFragments(Collection fragments) { getClasspathManager().loadFragments(fragments); } - private boolean lockClassName(String classname) { - synchronized (classNameLocks) { - Object lockingThread = classNameLocks.get(classname); - Thread current = Thread.currentThread(); - if (lockingThread == current) - return false; - boolean previousInterruption = Thread.interrupted(); - try { - while (true) { - if (lockingThread == null) { - classNameLocks.put(classname, current); - return true; - } - - classNameLocks.wait(); - lockingThread = classNameLocks.get(classname); - } - } catch (InterruptedException e) { - previousInterruption = true; - // must not throw LinkageError or ClassNotFoundException here because that will - // cause all threads - // to fail to load the class (see bug 490902) - throw new Error("Interrupted while waiting for classname lock: " + classname, e); //$NON-NLS-1$ - } finally { - if (previousInterruption) { - current.interrupt(); - } - } - } - } - - private void unlockClassName(String classname) { - synchronized (classNameLocks) { - classNameLocks.remove(classname); - classNameLocks.notifyAll(); - } + @Override + public Object getClassLoadingLock(String classname) { + return classNameLocks.getLock(classname); } public void close() {