WIP: Add support for java 9 by Sineaggi · Pull Request #1393 · OpenFeign/feign

Sineaggi · 2021-05-01T19:57:51Z

Attempt at fixing #935 via velo#4 and using multi-release jars.

velo

Having two copies of the sources is not an option... as nobody should be expected to patch two locations every time something changes.

Why do we need that?

velo · 2021-05-02T03:11:19Z

      - test:
-          jdk: '14-buster'
+          jdk: 'circleci/openjdk:14-buster'
          name: 'jdk 14'


I think as we add jdk 16 "support" we could drop 14.

I have an uncommitted change for this, moving to cimg/openjdk:16.0.0 (since that's where circleci appears to be migrating its images to).

Sineaggi · 2021-05-02T03:31:49Z

Having two copies of the sources is not an option... as nobody should be expected to patch two locations every time something changes.

Why do we need that?

Honestly just so I could get to fixing the tests. I'll be rewriting history to remove all the duplicate classes that are unnecessary when I can get maven to stop complaining.

Sineaggi · 2021-05-02T07:46:50Z

Unit tests are run via the compile output classpath, not via jar as per the maven lifecycle meaning the 'Multi-Release JAR Files' can't be used in tests. (As the multi-release jar doesn't exist until the package goal has run). That means we can't run java 16 unit-tests against this project, since java 16 doesn't like this code. (That's not entirely the truth, we could add --illegal-access=warn --add-opens java.base/java.lang.invoke=ALL-UNNAMED as flags to the tests to get them to pass, and then maybe add the failsafe plugin to integration test the jar after the fact, without those flags. Just an idea.) For now, I've removed the dependency on java 16 due to the added complexity around testing. This code really only needs to add the option to build against java9 as an additional supported release. In the future, I'd like to see maven's strategy around testing multi-release jar's improved, having to use integration-tests to test what seem to me to be unit's of code seems strange.

Sineaggi · 2021-05-02T07:48:12Z

Not sure yet how to handle the jdk8 builds. They don't like the --release, and can't compile the java 9 source. Maybe a profile, or remove the java 8 build?

Sineaggi · 2021-05-02T19:13:15Z

Looks like the build file currently releases most jars under java 8, and one jar under java 11. Wonder if it would be ok to move all the jars under 11, then use the --release to have java 11 cross compile to java 8.

velo · 2021-05-02T20:53:26Z

+ * method must be called on the result before invoke is called.
+ */
+@IgnoreJRERequirement
+final class DefaultMethodHandler implements MethodHandler {


Maybe we could add to https://github.com/OpenFeign/feign/tree/master/java11

I read mixed reviews of multi release support

Multi release has pretty sparse documentation from what I've found. It seems strange that there's no built-in functionality that would let your run unit-tests against the multi-release classes. The support for running tests against jars only exists in the failsafe plugin.

It might be possible to make a ServiceLocator that can load the different MethodHandlers, but that seems like a lot of complexity to add when using multi-release jars feels like it was built for exactly this purpose.

Yeah, having that on the java11 module instead, would lead to much better testability as well.

At least according to the gradle team, they don't recommend multi release jars according to this blog post. On the other hand, there's just one class that needs to be updated for java 9+, and we don't need a new api surface to support it, or jdk11 variant jars either. (The java 11 module looks like it just adds support for the new httpclient, and doesn't replace any existing functionality.)

Yeah, right now, java11 module only contain an http2 client. But nothing prevent from adding a new package with a better MethodHandler implementation.

Does java16 work at all with existing DefaultMethodHandler?

As you are finding out, trying to use the multi-release concept, even in theory, is difficult and wrought with drawbacks. We strive to keep complexity to a minimum. The simple if/else suggestion solves the problem without requiring us to support a multi-release infrastructure. This same advice is echoed by the Maven team themselves.

Libraries should make a decision based on: do I need this new Java feature? Can I make this Java version the new requirement? Would it be acceptable to solve this with an else/if-statement as mentioned in the first paragraph?

Given this change is isolated to the DefaultMethodHandler, I'd like to keep the changes small, even if we have, what amounts to a temporary if statement.

Are you thinking of maybe there being a class in the java11 package, similar to my branch above, but instead of going around with a full serviceloader and whatnot we instead use a Class.forName("DefaultMEthodHandlerJava11") and then fall back to 8 if unavailable? I don't exactly see the 'if/else statement' as being necessarily easier or less complex, it just shifts where the complexity is.
Have you looked at the other prototype branch above? It's not quite as simple as an if statement, but the locator nonesense could be removed in favor of something simpler. Would still require the java11 jar though.

I made an alternative change, with DefaultMethodHandler configurable
#1394

What do you thing of it?!

My only issue (which is specific to where I work) is for the most part clients ship their own instantiation of the Feign.builder() call, so end-users would have no ability to alter what method handler to use, making this not a viable solution.
EDIT: Might be specific to my work. Might also be rough for people who are supplied pre-existing clients, or where feign is an implementation concern, and not exposed as part of the api.

Also thinking about this further, this doesn't seem something pluggable in the same way jackson or the network library is imo, this is just about 'use this for one version of the jdk, this other thing for another version of the jdk'

velo · 2021-05-03T20:24:41Z

+  public DefaultMethodHandler(Method defaultMethod) {
+    try {
+      final Class<?> declaringClass = defaultMethod.getDeclaringClass();
+      final Lookup lookup = MethodHandles.privateLookupIn(declaringClass, MethodHandles.lookup());


I think I have a solution for this...

can we just use reflection on the existing DefaultMethodHandler?

We just need try to invoke the new API using reflection, if that fail, fallback to existing code

That’s what I’ve been saying

Will create a pr for this tonight

I just did, had some time allocated at work to be around feign =)

The changes look good! Really like the use of deny to get java 11 to help test this change.

…warnings

Sineaggi · 2021-05-03T22:28:13Z

+            <groupId>org.apache.maven.plugins</groupId>
+            <artifactId>maven-surefire-plugin</artifactId>
+            <configuration>
+              <argLine>--illegal-access=deny</argLine>


I like this!

velo

I'm happy with the current version of the change,

This also matches what @kdavisk6 suggested, thumbs up from me

kdavisk6 · 2021-05-03T22:57:08Z

💯

* Add support for java 9 * Move multi-release to java 9 only * Have a single DefaultMethodHandle for java 8 and 11 that don't print warnings * Bump spring version Co-authored-by: Clayton Walker <cwalker@sofi.org> Co-authored-by: Marvin Froeder <marvin.froeder@dovetailstudios.com>

Sineaggi force-pushed the add-multi-release-jar branch 5 times, most recently from 291e67d to 03417ca Compare May 2, 2021 00:46

velo requested changes May 2, 2021

View reviewed changes

Add support for java 9

88b07ca

Sineaggi force-pushed the add-multi-release-jar branch from 03417ca to 88b07ca Compare May 2, 2021 07:42

Sineaggi changed the title ~~WIP: Add support for java 16~~ WIP: Add support for java 9 May 2, 2021

Move multi-release to java 9 only

78e9924

velo reviewed May 2, 2021

View reviewed changes

velo reviewed May 3, 2021

View reviewed changes

velo added 2 commits May 4, 2021 09:48

Have a single DefaultMethodHandle for java 8 and 11 that don't print …

0e5a971

…warnings

Bump spring version

f34869a

Sineaggi commented May 3, 2021

View reviewed changes

velo approved these changes May 3, 2021

View reviewed changes

velo merged commit 3494a76 into OpenFeign:master May 3, 2021

Sineaggi deleted the add-multi-release-jar branch May 3, 2021 22:52

velo mentioned this pull request May 4, 2021

Illegal reflective access by feign.DefaultMethodHandler in Java 11 #935

Closed

Uh oh!

Conversation

Sineaggi commented May 1, 2021

Uh oh!

velo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sineaggi commented May 2, 2021

Uh oh!

Sineaggi commented May 2, 2021

Uh oh!

Sineaggi commented May 2, 2021

Uh oh!

Sineaggi commented May 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sineaggi May 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdavisk6 May 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

velo left a comment

Choose a reason for hiding this comment

Uh oh!

kdavisk6 commented May 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Sineaggi May 3, 2021 •

edited

Loading

kdavisk6 May 3, 2021 •

edited

Loading