Illegal reflective access by feign.DefaultMethodHandler in Java 11

[gavinws]

Similar to #393

This can be reproduced on Java 11 (probably also 9 and 10) with the Github example after renaming the package to notfeign.example.github (or anything that isn't under feign.*). Can be compiled with Java 8 or 11 (the compiler doesn't complain either way).

I reproduced on Zulu and Corretto 11.0.2 (all OpenJDK 11 implementations should behave the same):

$ java -version
openjdk version "11.0.2" 2019-01-15 LTS
OpenJDK Runtime Environment Zulu11.29+3-CA (build 11.0.2+7-LTS)
OpenJDK 64-Bit Server VM Zulu11.29+3-CA (build 11.0.2+7-LTS, mixed mode)

$ java -version
openjdk version "11.0.2" 2019-01-15 LTS
OpenJDK Runtime Environment Corretto-11.0.2.9.1 (build 11.0.2+9-LTS)
OpenJDK 64-Bit Server VM Corretto-11.0.2.9.1 (build 11.0.2+9-LTS, mixed mode)

$ java -jar target/feign-example-github-10.2.1-SNAPSHOT.jar
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by feign.DefaultMethodHandler (file:/Users/gavin/test/feign/example-github/target/feign-example-github-10.2.1-SNAPSHOT.jar) to field java.lang.invoke.MethodHandles$Lookup.IMPL_LOOKUP
WARNING: Please consider reporting this to the maintainers of feign.DefaultMethodHandler
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
Let's fetch and print a list of the contributors to this org.
[GitHub#repos] ---> GET https://api.github.com/users/openfeign/repos?sort=full_name HTTP/1.1
[GitHub#repos] <--- HTTP/1.1 200 OK (554ms)
...

Note that using the --illegal-access=warn JVM option just changes the warning to just the single line calling out feign.DefaultMethodHandler.

It seems like fixing this while maintaining compatibility for older Java versions will not be fun. Refs:

https://blog.jooq.org/2018/03/28/correct-reflective-access-to-interface-default-methods-in-java-8-9-10/

https://mydailyjava.blogspot.com/2018/04/jdk-11-and-proxies-in-world-past.html

[kdavisk6]

I'm pretty sure I know where this is. We manually bind any default methods to the proxy, which, I believe, was required previously. We will need to refactor that out to correct this issue.

Some background: When creating a proxy for an interface, we bind every method to the proxy. As a result, we needed to manually bind default implementations so they would still function. We will need to come up with a different solution for JDK 11 and beyond.

My first thought would be to update the processing of the interface to ignore all methods not annotated with @RequestLine, thereby, hopefully, removing the need to bind every method, but I'm not sure if that's going to work without breaking other libraries that have extended the default contract.

I'm going to mark this as an enhancement, with help-wanted and leave it open for others to think about and add to the discussion.

[velo]

I changed the module to run the jar after it's build...
#953

Locally it passes....
On travis, it fail due to API rate limit

[velo]

So, I was not able to reproduce this issue.

@gavinws can you look at my PR and indicate what am I missing?

[gavinws]

@velo you need to change the package names in the .java files to reproduce. Using feign.* as the base package name will not reproduce because it is not crossing module boundary (I think?). Hence I just quickly renamed them to notfeign.example.github in my test.

[velo]

OK, I renamed the packages and also include github tokens to my changes.

Let's see how it goes

[velo]

hi @gavinws
I change pages to be example.github... and did not encounter any problem.

Am I running it wrong?
https://github.com/OpenFeign/feign/pull/953/files#diff-1c45da44c7405f744cdc97bdfb851691R39

If you can create a unit test to reproduce this issue, it would be awesome

[gavinws]

I'm not sure why your test isn't working for this; it doesn't seem to capture output from running the jar or it doesn't run at all (I couldn't find the relevant output, but I could be blind). Also this warning does not interrupt execution or change the return code.

I just now stashed my changes, pulled master, and did a mvn clean install in the example-github directory and java -jar target/feign-example-github-10.3.0-SNAPSHOT.jar and reproduced the same warning when running the jar:

$ java -version
openjdk version "11.0.2" 2019-01-15 LTS
OpenJDK Runtime Environment Corretto-11.0.2.9.1 (build 11.0.2+9-LTS)
OpenJDK 64-Bit Server VM Corretto-11.0.2.9.1 (build 11.0.2+9-LTS, mixed mode)

$ pwd
/Users/gavin/test/feign/example-github

$ java -jar target/feign-example-github-10.3.0-SNAPSHOT.jar
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by feign.DefaultMethodHandler (file:/Users/gavin/test/feign/example-github/target/feign-example-github-10.3.0-SNAPSHOT.jar) to field java.lang.invoke.MethodHandles$Lookup.IMPL_LOOKUP
WARNING: Please consider reporting this to the maintainers of feign.DefaultMethodHandler
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
Let's fetch and print a list of the contributors to this org.
[GitHub#repos] ---> GET https://api.github.com/users/openfeign/repos?sort=full_name HTTP/1.1
...

Side note: I couldn't build the soap submodule because org.jvnet.mimepull:mimepull:jar:1.9.8 is missing from maven central.

[velo]

Side note: I couldn't build the soap submodule because org.jvnet.mimepull:mimepull:jar:1.9.8 is missing from maven central.

Odd, feign was built so many times, but you are right, this dependency does not exist

https://repo1.maven.org/maven2/org/jvnet/mimepull/mimepull/

[velo]

by no means I'm proposing this as a fix:
https://github.com/velo/feign/pull/4/files

Did some experimentation with DefaultMethodHandler.

I can't get it working on java 11.
Any suggestions?!

[gavinws]

I figured out how you can make your integration test fail, I think, in case you were still looking at how to make tests catch this. Using the --illegal-access=deny flag will cause an exception and exit:

$ java --illegal-access=deny -jar target/feign-example-github-10.3.0-SNAPSHOT.jar
Exception in thread "main" java.lang.reflect.InaccessibleObjectException: Unable to make field static final java.lang.invoke.MethodHandles$Lookup java.lang.invoke.MethodHandles$Lookup.IMPL_LOOKUP accessible: module java.base does not "opens java.lang.invoke" to unnamed module @49070868
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:340)
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:280)
	at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:176)
	at java.base/java.lang.reflect.Field.setAccessible(Field.java:170)
	at feign.DefaultMethodHandler.<init>(DefaultMethodHandler.java:41)
	at feign.ReflectiveFeign.newInstance(ReflectiveFeign.java:60)
	at feign.Feign$Builder.target(Feign.java:251)
	at feign.Feign$Builder.target(Feign.java:247)
	at example.github.GitHubExample$GitHub.connect(GitHubExample.java:75)
	at example.github.GitHubExample.main(GitHubExample.java:90)

I've also figured out that this can be worked around at runtime (hides the warning and passes the deny option above) with the --add-opens option, but who knows if this option will always be available (and it wouldn't be desirable for a project that properly uses modules):

java --illegal-access=warn --add-opens java.base/java.lang.invoke=ALL-UNNAMED -jar target/feign-example-github-10.3.0-SNAPSHOT.jar

For completeness I also tested this on Java 12 and the results are the same as Java 11 (as expected).

References:

• https://blog.codefx.org/java/five-command-line-options-hack-java-module-system/#Reflectively-Accessing-Internal-APIs-With--add-opens
• https://blog.codefx.org/java/java-9-migration-guide/#Illegal-Access-To-Internal-APIs

[velo]

So, there is no problem?!

[gavinws]

I wouldn't say there's no problem just because there is currently a workaround available. This still seems like something that should eventually be addressed as JDK restricts more things in the future.

[kdavisk6]

I've been looking into this more lately and it's something that we are going to need to, potentially, involve a larger group as it has implications on a lot of what this library does.

From what I can understand, JDK 9+ module security systems prevent access to objects that are effectively outside of the callers realm. In Feign, this occurs in our Proxy generation, specifically for dealing with default methods. I'll need to do more testing to see if there are other areas, but so far, default method handling appears to be main culprit. This leaves us with 2 options:

1. Stop supporting default method.
2. Find another solution.

Since I'm going to assume that dropping support for default methods is a unappropriate, I'm going to focus on what other solutions we can consider. For me, this becomes a question of do we keep the Proxy or not? If we decide that we want to keep the Proxy, we may want to consider replacing our JDK specific proxy generation with another library like ByteBuddy, which has a take on how to approach this issue and keep the Proxy like features.

Another is to forgo dynamic Proxy instances entirely and go with generated sources, similar to #990. This approach uses annotation processing to generate code that contains the implementation and represents a significant change in direction.

I'm sure there are/will be more creative options in the near future.

At this point, I don't really know where to go from here.

[kdavisk6]

This will be fixed as part of our next major release. In the meantime, this will continue to happen.

[krzyk]

Just a note, wiht JDK 16 release, that will be in March this won't be a warning but an error (so it will break the app, unless one adds a workaround).

[dpodder]

Another note, JEP 403 was announced recently as a candidate. Whenever it lands (which might even be JDK 17 at this point), --illegal-access=deny will be the default and all attempts to relax it will be ignored.

--add-opens should still be supported, at least for now.

[laech]

I can confirm that this crashes by default when running under JDK 16:

java.lang.reflect.InaccessibleObjectException: Unable to make field static final java.lang.invoke.MethodHandles$Lookup java.lang.invoke.MethodHandles$Lookup.IMPL_LOOKUP accessible: module java.base does not "opens java.lang.invoke" to unnamed module @3e6d8c1c
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:357)
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
	at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:177)
	at java.base/java.lang.reflect.Field.setAccessible(Field.java:171)
	at feign.DefaultMethodHandler.<init>(DefaultMethodHandler.java:41)
	at feign.ReflectiveFeign.newInstance(ReflectiveFeign.java:57)
	at feign.Feign$Builder.target(Feign.java:269)
	at org.springframework.cloud.openfeign.DefaultTargeter.target(DefaultTargeter.java:30)
	at org.springframework.cloud.openfeign.FeignClientFactoryBean.getTarget(FeignClientFactoryBean.java:381)
	at org.springframework.cloud.openfeign.FeignClientFactoryBean.getObject(FeignClientFactoryBean.java:339)
	at org.springframework.cloud.openfeign.FeignClientsRegistrar.lambda$registerFeignClient$0(FeignClientsRegistrar.java:230)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.obtainFromSupplier(AbstractAutowireCapableBeanFactory.java:1231)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBeanInstance(AbstractAutowireCapableBeanFactory.java:1173)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:564)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:524)
	... 116 common frames omitted

[krzyk]

And user won't be able to change that (at least easily) because illegal-access option is going away in JDK17 (https://blogs.oracle.com/javamagazine/java-runtime-encapsulation-internals ).

It would be possible with add-opens, but those need to be published somewhere for users to find and some will be reluctant to use them.

So it is a ticking bomb.

[kaspernielsen]

If you don't want people to fiddle with opening/exporting packages in module-info. Just add an extra builder method that lets them specify a lookup object when they are creating a builder. a.la.

Feign.builder(MethodHandles.lookup())

And use that lookup to unreflect the various methods.

Using open/export statements in module-info and letting people explicitly register Lookup objects are the two primary ways for support this kind of functionality in a modular world.

[velo]

Fixed by #1393