Test failures with OpenJDK17: probably ClassLoader evolutions since OpenJDK11

[pgrt]

Hello,

I am maintaining reflectasm in Debian. We have recently switched from OpenJDK11 to OpenJDK17 and this caused failures in the tests in com.esotericsoftware.reflectasm.ConstructorAccessTest (output at the end of my report).

I digged a bit, ran testHasProtectedConstructor and looked at loader1, loader2 and systemClassLoader in AccessClassLoader.areInSameRuntimeClassLoader called by ConstructorAccess.get. They are respectively
jdk.internal.loader.ClassLoaders$AppClassLoader@46fbb2c1, com.esotericsoftware.reflectasm.AccessClassLoader@42d3bd8b , and jdk.internal.loader.ClassLoaders$AppClassLoader@46fbb2c1
when using OpenJDK17, whereas with OpenJDK11 they are respectively
jdk.internal.loader.ClassLoaders$AppClassLoader@55054057, jdk.internal.loader.ClassLoaders$AppClassLoader@55054057, and jdk.internal.loader.ClassLoaders$AppClassLoader@55054057
when using OpenJDK11. Thus I suspect something has changed in the ClassLoader mechanism, perhaps you would like to investigate.

Also, in AccessClassLoader.areInSameRuntimeClassLoader, I think you would like to use .equals() methods instead of == as, e.g. in
if (type1.getPackage() == type2.getPackage()),
you are willing to compare values and not references.

Cheers,

Pierre

[INFO] Running com.esotericsoftware.reflectasm.ConstructorAccessTest
Unexpected exception happened: java.lang.RuntimeException: Class cannot be created (the no-arg constructor is protected or package-protected, and its ConstructorAccess could not be defined in the same class loader): com.esotericsoftware.reflectasm.ConstructorAccessTest$HasProtectedConstructor
Unexpected exception happened: java.lang.RuntimeException: Class cannot be created (the no-arg constructor is protected or package-protected, and its ConstructorAccess could not be defined in the same class loader): com.esotericsoftware.reflectasm.ConstructorAccessTest$HasPublicConstructor
Expected exception happened: java.lang.RuntimeException: Class cannot be created (missing no-arg constructor): com.esotericsoftware.reflectasm.ConstructorAccessTest$HasArgumentConstructor
Unexpected exception happened: java.lang.RuntimeException: Class cannot be created (the no-arg constructor is protected or package-protected, and its ConstructorAccess could not be defined in the same class loader): com.esotericsoftware.reflectasm.ConstructorAccessTest$HasPackageProtectedConstructor
Expected exception happened: java.lang.RuntimeException: Class cannot be created (the no-arg constructor is private): com.esotericsoftware.reflectasm.ConstructorAccessTest$HasPrivateConstructor
[ERROR] Tests run: 7, Failures: 3, Errors: 1, Skipped: 0, Time elapsed: 0.057 s <<< FAILURE! - in com.esotericsoftware.reflectasm.ConstructorAccessTest
[ERROR] testHasProtectedConstructor(com.esotericsoftware.reflectasm.ConstructorAccessTest) Time elapsed: 0.029 s <<< FAILURE!
junit.framework.AssertionFailedError
at com.esotericsoftware.reflectasm.ConstructorAccessTest.testHasProtectedConstructor(ConstructorAccessTest.java:74)

[ERROR] testHasPublicConstructor(com.esotericsoftware.reflectasm.ConstructorAccessTest) Time elapsed: 0.004 s <<< FAILURE!
junit.framework.AssertionFailedError
at com.esotericsoftware.reflectasm.ConstructorAccessTest.testHasPublicConstructor(ConstructorAccessTest.java:98)

[ERROR] testPackagePrivateNewInstance(com.esotericsoftware.reflectasm.ConstructorAccessTest) Time elapsed: 0.002 s <<< ERROR!
java.lang.RuntimeException: Class cannot be created (the no-arg constructor is protected or package-protected, and its ConstructorAccess could not be defined in the same class loader): com.esotericsoftware.reflectasm.ConstructorAccessTest$PackagePrivateClass
at com.esotericsoftware.reflectasm.ConstructorAccessTest.testPackagePrivateNewInstance(ConstructorAccessTest.java:31)

[ERROR] testHasPackageProtectedConstructor(com.esotericsoftware.reflectasm.ConstructorAccessTest) Time elapsed: 0.007 s <<< FAILURE!
junit.framework.AssertionFailedError
at com.esotericsoftware.reflectasm.ConstructorAccessTest.testHasPackageProtectedConstructor(ConstructorAccessTest.java:86)

[NathanSweet]

This evaluates to true:
https://github.com/EsotericSoftware/reflectasm/blob/master/src/com/esotericsoftware/reflectasm/ConstructorAccess.java#L108

It seems the difference from older JVMs is AccessClassLoader.areInSameRuntimeClassLoader(type, accessClass) is false with 17.

The if (type1.getPackage() != type2.getPackage()) { is right, they need to be the same package. Package doesn't override equals anyway.

As you found the classloaders are:

jdk.internal.loader.ClassLoaders$AppClassLoader@73d16e93
com.esotericsoftware.reflectasm.AccessClassLoader@1eb44e46

AccessClassLoader defineClass tries to use ClassLoader defineClass to define the class in the same class loader that loaded AccessClassLoader, but that is no longer allowed so execution continues past the try, defining the class in the AccessClassLoader. When the access class is defined that way, it does not have package private access and the test fails. This is expected behavior and I don't think there is a fix.

[pgrt]

Hello,

Thanks for looking at my issue. I had not noticed Package does not override equals.

Thanks for the detailed explanation on why the class loaders are different. Yet I am afraid I don't understand your conclusion: as the first lines of the log I posted suggest, we expect an exception only with HasPrivateConstructor and not for the other tests. Do you mean the tests should be reworked in order to be considered successful even if this exception is met with HasPackageProtectedConstructor, HasProtectedConstructor and HasPublicConstructor?

Best wishes,
Pierre

[NathanSweet]

Java 17 disallows defining a class from bytes in the class loader that loaded AccessClassLoader. That is required for the access class to have package private (aka "default access") member access. This causes ConstructorAccessTest testPackagePrivateNewInstance to fail. The test could be removed or reworked so it's skipped for Java >= 17.

Sorry I didn't notice the other test failures. I see them now:

testHasPublicConstructor: The constructor of the test class isn't actually public.

testHasPackageProtectedConstructor: Expected to fail with Java >= 17, which can't access package private members. I'd prefer this to be called "package private" or "default access".

testHasProtectedConstructor: Expected to fail with Java >= 17. Need to be in the same package to call a protected constructor. We could workaround this by subclassing, but that might be surprising.

[pgrt]

Hello,

Thanks for the detailed explanation and for the fix. It certainly answers my concerns.

Best,
Pierre

[gurka]

Hi @NathanSweet , sorry for bumping an old and closed issue, but I'm a bit unsure about a problem that we are seeing in our software, when upgrading to Java 17:

java.lang.IllegalAccessError:
class foo.bar.MyClassFieldAccess tried to access protected field foo.bar.MyClass.aField
(foo.bar.MyClassFieldAccess is in unnamed module of loader com.esotericsoftware.reflectasm.AccessClassLoader @7f608e21;
foo.bar.MyClass is in unnamed module of loader 'app')

Is the problem here the same as in testPackagePrivateNewInstance, i.e. that the two instances were created by different class loaders? Otherwise the access should have been OK?

Java 17 disallows defining a class from bytes in the class loader that loaded AccessClassLoader.

Are you saying that it is no longer possible to use the loader 'app', from the example/exception above, to define a class from bytes? In other words, you have to use a custom loader (like reflectasm.AccessClassLoader), which in turn means that package private access will never work?

[gurka]

I was able to get it work by updating reflectasm to use java.lang.invoke.MethodHandles.Lookup.defineClass instead of java.lang.ClassLoader.defineClass, as well as making it possible to provide a Lookup-instance. See https://github.com/gurka/reflectasm/commit/4f98000a76476cdfdd90c45b4567309141e79c08

This way, as long as I can provide a Lookup-instance from the foo.bar package, the class reflectasm creates can access protected fields in other classes in that package.

Does this change make sense? I think this is exactly what Byte Buddy did for Java 11+, as described here http://mydailyjava.blogspot.com/2018/04/jdk-11-and-proxies-in-world-past.html?m=1

[NathanSweet]

I don't have time at the moment to look deeper, but providing a Lookup is probably the way to go. Lookup has been around a long time (since Java 7), so it may not even break Java version compatibility (which is 7 but could be raised). If you'd like to make a PR, that'd be great! If passing Lookup is optional existing code should not break.

[gurka]

Lookup has been around a long time (since Java 7), so it may not even break Java version compatibility (which is 7 but could be raised).

From what I can see, currently the minimum version is 1.5. Also Lookup.defineClass was added in Java 9: https://docs.oracle.com/javase/9/docs/api/java/lang/invoke/MethodHandles.Lookup.html#defineClass-byte:A-

I'm not sure if you want to update the minimum version to 9?

Edit: created #92