Fix #47

[giamma]

Originally the static initialization was assuming that the ConstantPool
would be accessible via Class.getConstantPool(). However such method
only exists in the Oracle JDK and is not provided by the IBM OpenJ9 JDK.

But, both JDKs define an interface JavaLangAccess which offers method
getConstantPool( Class c ) that returns the constant pool for the given
class and the JavaLangAccess can be obtained via factory method
SharedSecrets.getJavaLangAccess().

Both SharedSecrets and JavaLangAccess have been relocated from sun.*
packages to jdk.internal.* packages in Java 9 or later.

Now:

• the code obtains the pool using the right classes from the right
  packages and the behavior is therefore portable across all JDKs
  available at the time of writing
• usage of Unsafe has been also removed
• Surefire plugin updated to be able to run TestNG tests.
• Surefire tests run in same JVM as maven to (forkedCount=0) to ensure
  that MAVEN_OPTS are propagated to test execution
• Travis file changed to also run tests on IBM OpenJ9 11.

[nfalco79]

This fixes the issue modelmapper/modelmapper#437

[jhalterman]

Very nice 👍

[jhalterman]

It seems that this change would now require users (in Java 9+?) to --add-exports? We should mention that clearly in the README and describe when --add-exports is needed, but overall I'm a bit hesitant to add such a restriction. Can you discuss the tradeoff and why this might be a good idea?

[nfalco79]

Interesting conversation related to this issue and the reason of the use of Unsafe class is in the #34

[giamma]

I don't think there is an alternative with JDK 10+ but I'd be happy to be proven wrong.

The original code was using Unsafe to make accessible getConstantPool of java.lang.Class, located in package java.lang, offered by module java.base which is by default accessible to every other module.

The new code to be portable and to work in JDK > 9 uses classes located in jdk.internal and this requires --add-exports. I am not aware of any way to avoid that switch for accessing jdk.internal.* classes but as I said I would be happy to be proven wrong.

Unsafe itself starting from JDK >= 10 is located in jdk.internal.* therefore unless we find a way to gain access to classes in such package without --add-exports there is little we can do.
And if we find such way, we don't need Unsafe anymore.

Considering that JDK 9 has already reached end of life, and that JDK 11 is the first long term support, the proposed pull request works the same as before on JDK 8 and works in the only supportable way on JDK >= 10.

What we could do is reinstate the old logic to behave as before when JDK is exactly 9, because in JDK 9 Unsafe is still located in com.sun.misc. However, I did not do that because nobody should be using JDK 9 as of now since it's unsupported by Oracle, cannot be downloaded for the desktop and as far as I know is also not supported by application servers. Also, to do so we would need to use a lot of reflection due to the fact that the package name is different in Java 10+ (sun.misc -> jdk.internal.misc)

[giamma]

In my workspace I changed the static block as follows to use the Java 9 JPMS API and a bit of reflection (I did not update the pull request):

      if (JAVA_VERSION < 9) {
          constantPoolName = "sun.reflect.ConstantPool";
          sharedSecretName = "sun.misc.SharedSecrets";
      } else {
          Module javaBase = ModuleLayer.boot().findModule("java.base").get();
          Method m = javaBase.getClass().getDeclaredMethod("implAddExportsOrOpens", String.class, Module.class, boolean.class, boolean.class);
          m.setAccessible(true);
          m.invoke(javaBase, "jdk.internal.reflect", TypeResolver.class.getModule(), false, true);
          m.invoke(javaBase, "jdk.internal.misc", TypeResolver.class.getModule(), false, true);
          
          constantPoolName = "jdk.internal.reflect.ConstantPool";
          sharedSecretName = "jdk.internal.misc.SharedSecrets";
      }

In short, it is getting the Module object for java.base and adding the required exports to make the packages visible to the module containing TypeResolver. There is a public method for adding the exports but it fails with a runtime error if invoked by a module other than java.base. However it delegates to a private method implAddExportsOrOpens so I tried calling it via reflection and it seems to work.

Unless I made a mistake, in Eclipse I can compile the code with Java 11 and tests are passing if I run them with either a Java8 launch configuration or a Java11 launch configuration. The class itself is produced in JDK8 format. I think this could be the case because all classes are in java.lang which is auto-imported so there is no reference to the Java9 classes in the bytecode before their actual use (i.e. no imports), which hopefully means that if the Java < 9 branch is taken their references are harmless. Worst case, one could write the same 3 lines of code via reflection.

Looks like a very dirty hack, not sure what could happen on Java Enterprise Application servers or in more complex scenarios.

Your thoughts?

[giamma]

As an additional comment, while the above does not require the --add-exports parameter I see the following in STDOUT:

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by net.jodah.typetools.TypeResolver (file:/git/typetools/target/classes/) to method java.lang.Module.implAddExportsOrOpens(java.lang.String,java.lang.Module,boolean,boolean)
WARNING: Please consider reporting this to the maintainers of net.jodah.typetools.TypeResolver
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

[nfalco79]

@jhalterman any comments?

[jhalterman]

Sorry for the delay...

I haven't looked closely at why this is, but the current tests do pass in master right now on JDK 11 (I just added a Travis check for this). Why might that be?

[nfalco79]

I haven't looked closely at why this is, but the current tests do pass in master right now on JDK 11 (I just added a Travis check for this). Why might that be?

From what I unsterstood the JDK 11 you enabled it's an OpenJDK instead the issue happens on "IBM OpenJ9 JDK"

[jhalterman]

Sure, which is why I'm hesitant to remove support for the current approach for Oracle/Open JDK users (most people) if not necessary.

[giamma]

If I am not mistaken, unsafe has been moved to a different package in jdk 10 and later so I don't understand how tests using lambdas can pass.

[nfalco79]

If I am not mistaken, unsafe has been moved to a different package in jdk 10 and later so I don't understand how tests using lambdas can pass.

With JDK 11, the first methods of sun.misc.Unsafe are being retired but the class is yet there.

The point here is modelmapper will not work for production (if lambas) for some application servers (for now seems IBM). For what I can see on modelmapper issue without a clear message in logs. This prevent the use of modelmapper for business libraries or at least with a double check for JDK compatibility.
This could be a good occasion to improve the travis pipeline and check the coverage for the most availables JDK on the market and declare the know limitations.

[nfalco79]

@jhalterman so the approach could be continue to use Usafe to access private field without the needs of --add-exports and switch to the proposed new approach to get the ConstantPool that is valid also for IBM JVM.
At this point since com.sun.Unsafe starting JDK11 (or maybe 9) is into jdk.unsupported module (that could not be always available) switch to jdk.internal.Unsafe in case of JAVA > 9

[jhalterman]

How about an adaptive approach - attempt to load the class with Unsafe, if it fails then go through SharedSecrets? That would minimize inconvenience for existing users so they don't need to --add-exports until the Unsafe APIs are actually removed in the future.

[nfalco79]

How about an adaptive approach - attempt to load the class with Unsafe, if it fails then go through SharedSecrets? That would minimize inconvenience for existing users so they don't need to --add-exports until the Unsafe APIs are actually removed in the future.

For what I understood com.sun.Unsafe was moved to jdk.internal.Unsafe to mark that it's a private package. The use of Unsafe is needed to access field or classes in jdk.internal package without the --add-exports due to the module system. SharedSecrets is the alternative to get the ConstantPool instead to get it directly throught jdk.internal.reflect.ConstantPool. This approach is valid on both JVM.

So @giamma in short if JAVA <= 8 than keep the actual approach instead if JAVA >= 9 than use jdk.internal.Usafe and SharedSecrets to get the ConstantPool right?

[jhalterman]

I would even keep the Unsafe approach for 10, 11 if it works, which for the Oracle and OpenJDKs I've tested with, it does.

[giamma]

Interesting reading for JDK 12 https://openjdk.java.net/jeps/334

[michelole]

FYI it seems jdk.internal.misc.SharedSecrets moved to jdk.internal.access.SharedSecrets in JDK12.