Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JUnit tests fail with InaccessibleObjectException with version 1.7 #393

Closed
duponter opened this issue Oct 19, 2022 · 21 comments
Closed

JUnit tests fail with InaccessibleObjectException with version 1.7 #393

duponter opened this issue Oct 19, 2022 · 21 comments
Labels

Comments

@duponter
Copy link

duponter commented Oct 19, 2022

Testing Problem

In our project we are running Junit tests that are using jqwik on the JPMS module path.
While on version 1.6.5, all these tests execute successfully.
However, after upgrading to version 1.7.0, several InaccessibleObjectExceptions are thrown during test execution:

java.lang.reflect.InaccessibleObjectException: 
Unable to make field private final java.util.function.Predicate java.util.function.Predicate$$Lambda$219/0x00000007c014e7b0.arg$1 accessible: 
module java.base does not "opens java.util.function" to module net.jqwik.api
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:354)
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
	at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:178)
	at java.base/java.lang.reflect.Field.setAccessible(Field.java:172)
	at net.jqwik.api@1.7.0/net.jqwik.api.support.LambdaSupport.fieldIsEqualIn(LambdaSupport.java:55)
	at net.jqwik.api@1.7.0/net.jqwik.api.support.LambdaSupport.areEqual(LambdaSupport.java:40)
	at net.jqwik.engine@1.7.0/net.jqwik.engine.properties.arbitraries.ArbitraryFilter.equals(ArbitraryFilter.java:48)
	at java.base/java.util.HashMap.getNode(HashMap.java:570)
	at java.base/java.util.LinkedHashMap.get(LinkedHashMap.java:441)
	at net.jqwik.engine@1.7.0/net.jqwik.engine.facades.SampleStreamFacade.getGenerator(SampleStreamFacade.java:38)
	at net.jqwik.engine@1.7.0/net.jqwik.engine.facades.SampleStreamFacade.lambda$getGeneratorForSampling$0(SampleStreamFacade.java:33)
	at net.jqwik.engine@1.7.0/net.jqwik.engine.execution.lifecycle.CurrentTestDescriptor.runWithDescriptor(CurrentTestDescriptor.java:17)
	at net.jqwik.engine@1.7.0/net.jqwik.engine.facades.SampleStreamFacade.runInDescriptor(SampleStreamFacade.java:60)
	at net.jqwik.engine@1.7.0/net.jqwik.engine.facades.SampleStreamFacade.getGeneratorForSampling(SampleStreamFacade.java:33)
	at net.jqwik.engine@1.7.0/net.jqwik.engine.facades.SampleStreamFacade.sampleStream(SampleStreamFacade.java:67)
	at net.jqwik.engine@1.7.0/net.jqwik.engine.facades.ArbitraryFacadeImpl.sampleStream(ArbitraryFacadeImpl.java:64)
	at net.jqwik.api@1.7.0/net.jqwik.api.Arbitrary.sampleStream(Arbitrary.java:387)
	at net.jqwik.api@1.7.0/net.jqwik.api.Arbitrary.sample(Arbitrary.java:419)

In the newly introduced LambdaSupport class, field values of incoming objects are read using reflection. This is a practice JPMS blocks to prevent unwanted access to a class's internals.
In this particular case, the JDK does not allow to inspect classes in the java.util.function package.

Suggested Solution

From a technical point of view, there are some ideas to solve this issue. However, I am not sure what the implications are for the jqwik functionalities that use the LambdaSupport class:

1. Avoid reflection

Avoid using reflection at all, rewrite the functionality using other techniques. This solution might break some use cases of the new jqwik functionality or requires to make some breaking changes, which you probably would like to avoid.

2. MethodHandles

Replace some reflection calls with their newer alternatives: MethodHandles and VarHandle. Although I have little to no experience with these alternatives, I quickly tried this approach:

private static boolean fieldIsEqualIn(Field field, Object left, Object right) {
    MethodHandles.Lookup lookup = MethodHandles.lookup();
    try {
        VarHandle handle = lookup.unreflectVarHandle(field);
        // If field is a functional type use LambdaSupport.areEqual().
        // TODO: Could there be circular references among functional types?
        if (isFunctionalType(field.getType())) {
            return areEqual(handle.get(left), handle.get(right));
        }
        return handle.get(left).equals(handle.get(right));
    } catch (IllegalAccessException e) {
        e.printStackTrace();
        return false;
    }
}

This resulted in a IllegalAccessException, but much closer to my own code:

java.lang.IllegalAccessException: class is not public: 
a.b.c.MyOwnProvider$$Lambda$718/0x00000007c02d64e8.arg$1/int/getField, 
from class net.jqwik.api.support.LambdaSupport (module net.jqwik.api)

With some additional experimenting, I am hopeful to make it work properly.

3. Update current implementation

Keep the current implementation, but prevent it from crawling into the internals of the JDK.

Discussion

Until now, I only performed some superficial research of this issue. If time permits, I'll try to provide a reproducible test setup.
Also, I will try to dig a little deeper into the second solution (MethodHandles).
For now, we will stay on version 1.6.5.

@jlink
Copy link
Collaborator

jlink commented Oct 19, 2022

@duponter Thanks for catching that.

To be frank, my knowledge about Java’s module system is very limited. I’ll ask my personal module system liaison if they have a good idea about how to solve that problem. Apart from that, I’d very much appreciate if you’re approach turns out to work.

@jlink
Copy link
Collaborator

jlink commented Oct 19, 2022

@sormuras Any quick idea how to tackle this?

@sormuras
Copy link
Contributor

sormuras commented Oct 19, 2022

Assuming that the code under test is also in a module, it should permit deep reflection for jqwik into some of its packages.

Like:

module a.b.c {
  // ...
  requires net.jqwik.api;
  // ...
  opens a.b.c to
    net.jqwik.api;
}

I usually use open module to permit all (test) frameworks to reflect into my test modules. Don't do this with main modules.

@osi
Copy link
Contributor

osi commented Oct 19, 2022

I'm also seeing this:

java.lang.reflect.InaccessibleObjectException: Unable to make field private final java.util.function.Predicate java.util.function.Predicate$$Lambda$205/0x00000008010c86b0.arg$1 accessible: module java.base does not "opens java.util.function" to unnamed module @646be2c3

	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:354)
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
	at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:178)
	at java.base/java.lang.reflect.Field.setAccessible(Field.java:172)
	at net.jqwik.api.support.LambdaSupport.fieldIsEqualIn(LambdaSupport.java:55)
	at net.jqwik.api.support.LambdaSupport.areEqual(LambdaSupport.java:40)
	at net.jqwik.engine.properties.arbitraries.ArbitraryFilter.equals(ArbitraryFilter.java:48)
	at java.base/java.util.AbstractList.equals(AbstractList.java:548)
	at net.jqwik.engine.properties.arbitraries.CombineArbitrary.equals(CombineArbitrary.java:62)
	at java.base/java.util.Objects.equals(Objects.java:64)
	at net.jqwik.api.Tuple$Tuple3.equals(Tuple.java:235)
	at java.base/java.util.HashMap.getNode(HashMap.java:570)
	at java.base/java.util.LinkedHashMap.get(LinkedHashMap.java:441)
	at net.jqwik.engine.facades.Memoize.computeIfAbsent(Memoize.java:44)
	at net.jqwik.engine.facades.Memoize.memoizedGenerator(Memoize.java:29)
	at net.jqwik.engine.facades.ArbitraryFacadeImpl.memoizedGenerator(ArbitraryFacadeImpl.java:182)
	at net.jqwik.api.Arbitrary.generator(Arbitrary.java:96)
	at net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.lambda$new$0(FlatMappedShrinkable.java:26)
	at net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.lambda$new$1(FlatMappedShrinkable.java:31)
	at net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.generateShrinkable(FlatMappedShrinkable.java:40)
	at net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.shrinkable(FlatMappedShrinkable.java:84)
	at net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.value(FlatMappedShrinkable.java:80)
	at net.jqwik.engine.properties.state.ShrinkableChainIteration.transformation(ShrinkableChainIteration.java:64)
	at net.jqwik.engine.properties.state.ShrinkableChain$ChainInstance.lambda$transformations$0(ShrinkableChain.java:114)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
	at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1625)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
	at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:921)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:682)
	at net.jqwik.engine.properties.state.ShrinkableChain$ChainInstance.transformations(ShrinkableChain.java:114)
	at net.jqwik.engine.properties.state.SequentialActionChain.transformations(SequentialActionChain.java:30)
	at net.jqwik.engine.properties.state.SequentialActionChain.toString(SequentialActionChain.java:127)
	at net.jqwik.engine.support.JqwikStringSupport.displayString(JqwikStringSupport.java:37)
	at net.jqwik.engine.execution.reporting.ObjectValueReport.toStringLines(ObjectValueReport.java:17)
	at net.jqwik.engine.execution.reporting.ObjectValueReport.<init>(ObjectValueReport.java:13)
	at net.jqwik.engine.execution.reporting.ValueReport.of(ValueReport.java:54)
	at net.jqwik.engine.execution.reporting.ValueReport.of(ValueReport.java:26)
	at net.jqwik.engine.execution.reporting.ValueReport.of(ValueReport.java:21)
	at net.jqwik.engine.execution.reporting.SampleReporter.reportParameters(SampleReporter.java:69)
	at net.jqwik.engine.execution.reporting.SampleReporter.reportTo(SampleReporter.java:62)
	at net.jqwik.engine.execution.reporting.SampleReporter.reportSample(SampleReporter.java:24)
	at net.jqwik.engine.execution.reporting.ExecutionResultReport.reportParameterChanges(ExecutionResultReport.java:118)
	at net.jqwik.engine.execution.reporting.ExecutionResultReport.lambda$appendSamples$2(ExecutionResultReport.java:83)
	at java.base/java.util.Optional.ifPresent(Optional.java:178)
	at net.jqwik.engine.execution.reporting.ExecutionResultReport.appendSamples(ExecutionResultReport.java:77)
	at net.jqwik.engine.execution.reporting.ExecutionResultReport.buildJqwikReport(ExecutionResultReport.java:55)
	at net.jqwik.engine.execution.reporting.ExecutionResultReport.from(ExecutionResultReport.java:35)
	at net.jqwik.engine.execution.PropertyMethodExecutor.reportResult(PropertyMethodExecutor.java:102)
	at net.jqwik.engine.execution.PropertyMethodExecutor.execute(PropertyMethodExecutor.java:59)
	at net.jqwik.engine.execution.PropertyTaskCreator.executeTestMethod(PropertyTaskCreator.java:166)
	at net.jqwik.engine.execution.PropertyTaskCreator.lambda$createTask$1(PropertyTaskCreator.java:51)
	at net.jqwik.engine.execution.lifecycle.CurrentDomainContext.runWithContext(CurrentDomainContext.java:28)
	at net.jqwik.engine.execution.PropertyTaskCreator.lambda$createTask$2(PropertyTaskCreator.java:50)
	at net.jqwik.engine.execution.pipeline.ExecutionTask$1.lambda$execute$0(ExecutionTask.java:31)
	at net.jqwik.engine.execution.lifecycle.CurrentTestDescriptor.runWithDescriptor(CurrentTestDescriptor.java:17)
	at net.jqwik.engine.execution.pipeline.ExecutionTask$1.execute(ExecutionTask.java:31)
	at net.jqwik.engine.execution.pipeline.ExecutionPipeline.runToTermination(ExecutionPipeline.java:82)
	at net.jqwik.engine.execution.JqwikExecutor.execute(JqwikExecutor.java:46)
	at net.jqwik.engine.JqwikTestEngine.executeTests(JqwikTestEngine.java:70)
	at net.jqwik.engine.JqwikTestEngine.execute(JqwikTestEngine.java:53)

@jlink
Copy link
Collaborator

jlink commented Oct 20, 2022

Assuming that the code under test is also in a module, it should permit deep reflection for jqwik into some of its packages.

@duponter @osi Does opening your module under test to jqwik resolve the problems? Is that an acceptable solution?

@sormuras
Copy link
Contributor

@jlink You might want to consider accepting an instance of java.lang.invoke.MethodHandles.Lookup in LambdaSupport's methods. This way an author of a test module may create and pass an instance with sufficient access rights.

@jlink
Copy link
Collaborator

jlink commented Oct 20, 2022

As a workaround, I could ignore InaccessibleObjectExceptions when thrown within LambdaSupport and assume lambdas are not equal. This would result in less memoization of generators, thereby degrading generation performance.

In addition, a warning could be logged: "Please add opens <your project id> to net.jqwik.api; to your module-info file".

@jlink
Copy link
Collaborator

jlink commented Oct 20, 2022

I failed to reproduce the problem locally. Can anyone provide a failing sample?

@sormuras
Copy link
Contributor

@osi posted a stacktrace with jqwik being on the class path:

java.lang.reflect.InaccessibleObjectException: Unable to make field private final java...

	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:354)
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
	at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:178)
	at java.base/java.lang.reflect.Field.setAccessible(Field.java:172)
	at net.jqwik.api.support.LambdaSupport.fieldIsEqualIn(LambdaSupport.java:55)
	at net.jqwik.api.support.LambdaSupport.areEqual(LambdaSupport.java:40)
	at net.jqwik.engine.properties.arbitraries.ArbitraryFilter.equals(ArbitraryFilter.java:48)
	...

Note the missing net.jqwik.api@1.7.0/ and net.jqwik.engine@1.7.0/... prefixes.

@duponter
Copy link
Author

I have a working solution, based on my own assumptions and the suggestions by @jlink...

As a workaround, I could ignore InaccessibleObjectExceptions when thrown within LambdaSupport and assume lambdas are not equal. This would result in less memoization of generators, thereby degrading generation performance.

In addition, a warning could be logged: "Please add opens <your project id> to net.jqwik.api; to your module-info file".

... and @sormuras

@jlink You might want to consider accepting an instance of java.lang.invoke.MethodHandles.Lookup in LambdaSupport's methods. This way an author of a test module may create and pass an instance with sufficient access rights.

When changing the implementation of LambdaSupport.fieldIsEqualIn like illustrated below (might open a PR later), my tests are succeeding again.

private static final MethodHandles.Lookup LOOKUP = MethodHandles.lookup();

private static boolean fieldIsEqualIn(Field field, Object left, Object right) {. 
    try {
        MethodHandle handle = LOOKUP.unreflectGetter(field);
        // If field is a functional type use LambdaSupport.areEqual().
        // TODO: Could there be circular references among functional types?
        if (isFunctionalType(field.getType())) {
	    return areEqual(handle.invoke(left), handle.invoke(right));
	}
	return handle.invoke(left).equals(handle.invoke(right));
    } catch (Throwable e) {
	return false;
    }
}

However, there are some important remarks/comments to consider:

  1. While my test module is (and always was) open to jqwik-api, the java.base module is not. Hence, the InaccessibleObjectException thrown when trying to access fields in the java.util.function package.
  2. In the current implementation, field.setAccessible(true) throws the java.lang.reflect.InaccessibleObjectException, but in my fix java.lang.IllegalAccessExceptions are thrown, with the only difference that they're being swallowed by the Throwable catch clause:
java.lang.IllegalAccessException: class is not public:
java.util.function.Predicate$$Lambda$203/0x00000007c0149250.arg$1/java.util.function.Predicate/getField, from class net.jqwik.api.support.LambdaSupport (module net.jqwik.api)
  1. Finally, as @jlink mentioned, there is a performance hit when a lot of exceptions are created and when there is less memoization of generators. Maybe there is a way to prevent these exceptions from being created? Or by preventing crawling into the JDK's base classes?

@osi
Copy link
Contributor

osi commented Oct 20, 2022

@jlink my code isn't in a module; i agree with @duponter 's assessment that it is the module for the JDK itself that needs to be opened.

i also agree with the workaround for not-failing when this happens but letting the user know how they can remediate (and speed their tests)

@jlink
Copy link
Collaborator

jlink commented Oct 20, 2022

@jlink my code isn't in a module;

So it's failing due to a newer Java version that doesn't allow this kind of reflection any more?

i also agree with the workaround for not-failing when this happens but letting the user know how they can remediate (and speed their tests)

If opening the module under test for jqwik does not suffice (as implied above), there is no remediation, or is there?

@duponter
Copy link
Author

So it's failing due to a newer Java version that doesn't allow this kind of reflection any more?

True, basically as of Java 9+.

If opening the module under test for jqwik does not suffice (as implied above), there is no remediation, or is there?

Use the remediation I posted earlier, it compiles on JDK 8:

private static final MethodHandles.Lookup LOOKUP = MethodHandles.lookup();

private static boolean fieldIsEqualIn(Field field, Object left, Object right) {. 
    try {
        MethodHandle handle = LOOKUP.unreflectGetter(field);
        // If field is a functional type use LambdaSupport.areEqual().
        // TODO: Could there be circular references among functional types?
        if (isFunctionalType(field.getType())) {
	    return areEqual(handle.invoke(left), handle.invoke(right));
	}
	return handle.invoke(left).equals(handle.invoke(right));
    } catch (Throwable e) {
	return false;
    }
}

I am afraid these problems will occur more often in the future as applications start using recent JDK versions more frequently, either with or without the Java Module system.
A random poll at Devoxx Belgium last week revealed that approximately 25% of the attendants uses JDK 17 in production, 50+% JDK 11 and less than 25% are still on JDK 8.
Maybe you should consider migrating the library to JDK 11+ and provide multi release jars for backwards compatibility with JDK 8. I agree this might be challenging.

@jlink
Copy link
Collaborator

jlink commented Oct 20, 2022

I still don’t really understand. I build and run with JDKs 8, 11, 17 and 18 and the problem doesn’t show up in my test suite. That’s why I’d really like to be able to reproduce it. Can you provide a minimal example?

@duponter
Copy link
Author

With the help of a colleague, I finally managed to reproduce the stacktrace with a minimal example.

It has nothing to do with Java Platform Modules. In hindsight it seems logical as @sormuras already pointed out on the stacktrace posted by @osi that there is a difference (no versions vs lib versions shown) in the stacktraces produced on the classpath and those on the modulepath.

Instead, the issue is caused by a combination of the Arbitrary equality check logic and the use of java.util.Predicate#not in Arbitrary.filter. Here are some plain JUnit 5 tests to reproduce it (running on JDK 17):

@Test
void test1FailsBecauseDifferentArbitraryInstancesWithSameDataAndNegated() {
    String first = Arbitraries.of("NL", "DE", "BE").filter(not(isoCode -> "BE".equals(isoCode))).sample();
    String second = Arbitraries.of("NL", "DE", "BE").filter(not(isoCode -> "BE".equals(isoCode))).sample();
}

@Test
void test2SucceedsBecauseDifferentArbitraryInstancesWithSameDataButNotNegated() {
    String first = Arbitraries.of("NL", "DE", "BE").filter(isoCode -> "BE".equals(isoCode)).sample();
    String second = Arbitraries.of("NL", "DE", "BE").filter(isoCode -> "BE".equals(isoCode)).sample();
}

@Test
void test3SucceedsBecauseArbitraryInstancesWithDifferentData() {
    String first = Arbitraries.of("DE", "BE", "FR").filter(not(isoCode -> "BE".equals(isoCode))).sample();
    String second = Arbitraries.of("DE", "BE").filter(not(isoCode -> "BE".equals(isoCode))).sample();
}

@Test
void test4SucceedsBecauseSameArbitraryInstanceIsReused() {
    Arbitrary<String> arbitrary = Arbitraries.of("BR", "AR", "MX").filter(not(isoCode -> "BE".equals(isoCode)));
    String first = arbitrary.sample();
    String second = arbitrary.sample();
}

When Arbitrary's wrap the same data (but are not the same instance) en is afterwards filtered using Predicate.not, the aforementioned InaccessibleObjectException is thrown.
Hopefully this is sufficient for you, @jlink, to be able to fix the issue.

@jlink
Copy link
Collaborator

jlink commented Oct 21, 2022

@duponter Thanks a lot. That gives me a good start!

@jlink jlink added the bug label Oct 21, 2022
jlink added a commit that referenced this issue Oct 21, 2022
@jlink
Copy link
Collaborator

jlink commented Oct 21, 2022

Fixed in 747bc44

I decided to not log if the reflection exception shows up, because there's not really a lot people can do, and in most cases, it won't affect performance anyway.

@jlink
Copy link
Collaborator

jlink commented Oct 21, 2022

The fix is present in 1.7.1-SNAPSHOT. Please try it out.

@jlink jlink closed this as completed Oct 21, 2022
@duponter
Copy link
Author

The SNAPSHOT version fixes the issue. As mentioned in the commit:

When I tested this solution, I did not need field.setAccessible(true); anymore. I think it can be deleted.

@duponter
Copy link
Author

duponter commented Nov 4, 2022

@jlink After upgrading to the freshly released 1.7.1 version, all tests complete successfully.
However, I had to increase the Xmx of the surefire plugin (from 1024m to 1536m) to avoid Java heap issues. While this poses no additional issues for me (and might as well be caused by my local setup), it is worth a mention imho.

@jlink
Copy link
Collaborator

jlink commented Nov 4, 2022

@duponter The additional heap size might be a coincidence (there's randomness involved) or it could be related. Maybe you can report when it turns out - after more samples - that it's really related.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants