Skip to content

Commit

Permalink
Get loader-classname lock for calls to findLoadedClass
Browse files Browse the repository at this point in the history
This is related to Open J9 issue
eclipse-openj9/openj9#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.
  • Loading branch information
tjwatson committed Nov 20, 2024
1 parent 2815608 commit 77990e6
Show file tree
Hide file tree
Showing 3 changed files with 111 additions and 85 deletions.
Original file line number Diff line number Diff line change
@@ -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<Key, Lock> {

final ReferenceQueue<Lock> refQueue = new ReferenceQueue<>();
private final ConcurrentHashMap<Key, LockWeakRef> lockMap = new ConcurrentHashMap<>();
private final Function<Key, Lock> lockCreator;

public KeyBasedLockStore(Function<Key, Lock> lockCreator) {
this.lockCreator = lockCreator;
}

private final class LockWeakRef extends WeakReference<Lock> {
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);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.
Expand All @@ -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
Expand All @@ -69,11 +63,6 @@ public final ClasspathManager getClasspathManager() {
return manager;
}

@Override
public final boolean isRegisteredAsParallel() {
return isRegisteredAsParallel;
}

@Override
public final BundleLoader getBundleLoader() {
return delegate;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -100,7 +97,26 @@ public DefineClassResult(Class<?> clazz, boolean defined) {
}
}

private final Map<String, Thread> classNameLocks = new HashMap<>(5);
private static final class ClassNameLock {
static final Function<String, ClassNameLock> SUPPLIER = new Function<String, ClassNameLock>() {
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<String, ClassNameLock> classNameLocks = new KeyBasedLockStore<>(
ClassNameLock.SUPPLIER);
private final Object pkgLock = new Object();

/**
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -427,42 +426,9 @@ public void loadFragments(Collection<ModuleRevision> 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() {
Expand Down

0 comments on commit 77990e6

Please sign in to comment.