The locking behavior within the ByteArrayClassLoader.ChildFirst#isShadowed method

[dogourd]

 /**
 * Checks if a resource name represents a class file of a class that was loaded by this class loader.
 *
 * @param resourceName The resource name of the class to be exposed as its class file.
 * @return {@code true} if this class represents a class that is being loaded by this class loader.
 */
private boolean isShadowed(String resourceName) {
    if (persistenceHandler.isManifest() || !resourceName.endsWith(CLASS_FILE_SUFFIX)) {
        return false;
    }
    // This synchronization is required to avoid a racing condition to the actual class loading.
    synchronized (this) {
        String typeName = resourceName.replace('/', '.').substring(0, resourceName.length() - CLASS_FILE_SUFFIX.length());
        if (typeDefinitions.containsKey(typeName)) {
            return true;
        }
        Class<?> loadedClass = findLoadedClass(typeName);
        return loadedClass != null && loadedClass.getClassLoader() == this;
    }
}

Can the locking part use SynchronizationStrategy to avoid locking the entire classloader instance? For Example:

String typeName = resourceName.replace('/', '.').substring(0, resourceName.length() - CLASS_FILE_SUFFIX.length());
if (typeDefinitions.containsKey(typeName)) {
    return true;
}
synchronized (SYNCHRONIZATION_STRATEGY.initialize().getClassLoadingLock(this, typeName)) {
    Class<?> loadedClass = findLoadedClass(typeName);
    return loadedClass != null && loadedClass.getClassLoader() == this;
}

The problem with locking the classLoader instance is that if the subclass implementation is not declared as parallel capable, meaning it hasn't called ClassLoader#registerAsParallelCapable, then loadClass and getResource are more likely to trigger deadlocks, even if they are not operating on the same resource.
Because I noticed that this part of the locking code seems to have been present since ByteBuddy introduced the SynchronizationStrategy API, I am not sure if this is an oversight.

[dogourd]

I wrote a simple example to reproduce this. In my test case, if the custom class loader does not call registerAsParallelCapable, the program almost always deadlocks. On the other hand, if it does call registerAsParallelCapable, the program finishes normally.

public class BAClassLoaderTest {

    private static class Foo {
    }

    private static class Bar {
    }

    public static void main(String[] args) throws Throwable {
        ClassLoader systemClassLoader = ClassLoader.getSystemClassLoader();
        ClassFileLocator systemClassFileLocator = ClassFileLocator.ForClassLoader.of(systemClassLoader);
        String[] classes = {Foo.class.getName(), Bar.class.getName()};
        Map<String, byte[]> classBytes = new HashMap<String, byte[]>();
        for (String klass : classes) {
            byte[] byteRepresentation = systemClassFileLocator.locate(klass).resolve();
            classBytes.put(klass, byteRepresentation);
        }

        ByteArrayClassLoader.ChildFirst childFirst = new ByteArrayClassLoader.ChildFirst(
                systemClassLoader, classBytes,
                BAClassLoaderTest.class.getProtectionDomain(),
                ByteArrayClassLoader.PersistenceHandler.LATENT,
                PackageDefinitionStrategy.Trivial.INSTANCE
        ) {
            static {
                //registerAsParallelCapable();
            }
        };

        Class<?> bar = childFirst.loadClass(Bar.class.getName());


        Instrumentation inst = ByteBuddyAgent.install();

        Object agentGlobalLock = new Object();
        new AgentBuilder.Default()
                .with(AgentBuilder.RedefinitionStrategy.RETRANSFORMATION)
                .with(new AgentBuilder.RedefinitionStrategy.DiscoveryStrategy.Explicit())
                .disableClassFormatChanges()
                .type(namedOneOf(Foo.class.getName(), Bar.class.getName()))
                .transform((builder, typeDescription, classLoader, module, protectionDomain) -> {
                    String classname = typeDescription.getName();
                    // add breakpoint on classname.contains(Foo.class.getName());
                    // t1 holds child-first lock and waiting for agentGlobalLock.
                    synchronized (agentGlobalLock) {
                        try {
                            // add breakpoint on classname.contains(Bar.class.getName());
                            // t2 holds agentGlobalLock and waiting for child-first lock
                            getResource(childFirst, classname);
                        } catch (Exception e) {
                            e.printStackTrace();
                        }
                    }
                    return builder;
                }).installOn(inst);


        Thread t1 = new Thread(() -> {
            try {
                // get ClassLoadingLock and then try to get agentGlobalLock in ClassFileTransformer.
                loadClass(childFirst, Foo.class.getName());
            } catch (Throwable e) {
                e.printStackTrace();
            }
        }, "t1");
        Thread t2 = new Thread(() -> {
            try {
                // get agentGlobalLock in ClassFileTransformer and then try to get classLoader lock in ChildFirst#isShadow.
                inst.retransformClasses(bar);
            } catch (Throwable e) {
                e.printStackTrace();
            }
        }, "t2");

        t1.start();
        t2.start();

        t1.join();
        t2.join();


        System.out.println("finished");
    }

    private static Class<?> loadClass(ByteArrayClassLoader.ChildFirst childFirst, String classname)
            throws ClassNotFoundException {
        return childFirst.loadClass(classname);
    }

    private static void getResource(ByteArrayClassLoader.ChildFirst childFirst, String classname) {
        String resource = classname.replace('.', '/') + ".class";
        childFirst.getResource(resource);
    }
}

[raphw]

Indeed, this is an overlook. Fixed it now.