Java 9 support: use Unsafe-based reflection in Java 9+ #1218

amogilev · 2018-01-01T07:37:53Z

Fixes "illegal reflective access" warnings and exceptions, like one in #1216

fixes "illegal reflective access" warnings and exceptions

inder123 · 2018-01-02T17:13:45Z

gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java

 import com.google.gson.reflect.TypeToken;

 /**
 * Returns a function that can construct an instance of a requested type.
 */
 public final class ConstructorConstructor {
  private final Map<Type, InstanceCreator<?>> instanceCreators;
+  private final static ReflectionAccessor accessor = ReflectionAccessUtils.getReflectionAccessor();


This doesn't need to be static. There is just one instance of ConstructorConstructor in Gson

inder123 · 2018-01-02T17:14:17Z

gson/src/main/java/com/google/gson/internal/bind/ReflectiveTypeAdapterFactory.java

@@ -49,6 +51,7 @@
  private final FieldNamingStrategy fieldNamingPolicy;
  private final Excluder excluder;
  private final JsonAdapterAnnotationTypeAdapterFactory jsonAdapterFactory;
+  private final static ReflectionAccessor accessor = ReflectionAccessUtils.getReflectionAccessor();


Again doesn't need to be static

inder123 · 2018-01-02T17:15:25Z

gson/src/main/java/com/google/gson/reflect/ReflectionAccessUtils.java

+ * <p/>
+ * Works both for Java 9 and earlier Java versions.
+ */
+public class ReflectionAccessUtils {


Can you delete this class, and move this code to a getInstance() method under ReflectionAccessor?

inder123 · 2018-01-02T17:16:20Z

gson/src/main/java/com/google/gson/reflect/ReflectionAccessor.java

+ * Provides a replacement for {@link AccessibleObject#setAccessible(boolean)}, useful when that basic operation is
+ * prohibited, e.g. throws {@link java.lang.reflect.InaccessibleObjectException} in Java 9.
+ */
+public interface ReflectionAccessor {


This can be made an abstract class, no need of interface

inder123 · 2018-01-02T17:16:35Z

gson/src/main/java/com/google/gson/reflect/impl/PreJava9ReflectionAccessor.java

+ * This implementation just calls {@link AccessibleObject#setAccessible(boolean) setAccessible(true)}, which worked
+ * fine before Java 9.
+ */
+public class PreJava9ReflectionAccessor implements ReflectionAccessor {


make final?

inder123 · 2018-01-02T17:17:03Z

gson/src/main/java/com/google/gson/reflect/impl/UnsafeReflectionAccessor.java

+ * NOTE: This implementation is designed for Java 9. Although it should work with earlier Java releases, it is better to
+ * use {@link PreJava9ReflectionAccessor} for them.
+ */
+public class UnsafeReflectionAccessor implements ReflectionAccessor {


inder123 · 2018-01-02T17:17:17Z

gson/src/main/java/com/google/gson/reflect/impl/UnsafeReflectionAccessor.java

+ */
+public class UnsafeReflectionAccessor implements ReflectionAccessor {
+
+  private static final Unsafe theUnsafe = getUnsafeInstance();


remove static? There will be one instance of this class anyway

inder123 · 2018-01-02T17:18:08Z

Thanks for the PR

JakeWharton · 2018-01-02T18:58:55Z

All of these classes should be in internal. They're not public API.

amogilev · 2018-01-02T19:03:16Z

@inder123 Thanks for the review, I have changed the code according to your comments.
@JakeWharton Do you mean that the code shall be moved to the package 'com.google.gson.internal.reflect'? Guess it makes sense...

liach · 2018-01-02T19:28:52Z

gson/src/main/java/com/google/gson/reflect/package-info.java

@@ -1,5 +1,6 @@
 /**
- * This package provides utility classes for finding type information for generic types.
+ * This package provides utility classes for finding type information for generic types,
+ * as well as  some utilities for working with Reflection API.


two spaces between as and some

inder123 · 2018-01-02T22:46:51Z

gson/src/main/java/com/google/gson/reflect/ReflectionAccessor.java

+
+  // singleton holder
+  private static class ReflectionAccessorHolder {
+    private static final ReflectionAccessor instance = createReflectionAccessor();


can you get rid of ReflectionAccessorHolder class, and just place the instance in ReflectionAccessor?

If you get the holder, the instance field should not be private because of synthetic method (jvm does not allow such access so that java will generate extra methods to this field)

@inder123 You are right, there is no need of laziness for this singleton, so the holder can be removed

inder123 · 2018-01-02T22:47:06Z

gson/src/main/java/com/google/gson/reflect/impl/PreJava9ReflectionAccessor.java

@@ -25,7 +25,7 @@
 * This implementation just calls {@link AccessibleObject#setAccessible(boolean) setAccessible(true)}, which worked
 * fine before Java 9.
 */
-public class PreJava9ReflectionAccessor implements ReflectionAccessor {
+public final class PreJava9ReflectionAccessor extends ReflectionAccessor {


This class can be package private

inder123 · 2018-01-02T22:47:13Z

gson/src/main/java/com/google/gson/reflect/impl/UnsafeReflectionAccessor.java

@@ -27,7 +27,7 @@
 * NOTE: This implementation is designed for Java 9. Although it should work with earlier Java releases, it is better to
 * use {@link PreJava9ReflectionAccessor} for them.
 */
-public class UnsafeReflectionAccessor implements ReflectionAccessor {
+public final class UnsafeReflectionAccessor extends ReflectionAccessor {


package private?

It implies that 'reflect.impl' classes need to be moved up to the 'reflect' package. Seems fine after moving to a separate 'internal' package though...

liach · 2018-01-02T23:17:54Z

gson/src/main/java/com/google/gson/reflect/ReflectionAccessor.java

+ * <p/>
+ * Works both for Java 9 and earlier Java versions.
+ */
+public abstract class ReflectionAccessor {


package-private constructor please

Why adding a constructor here? The class is abstract, so the default (public) constructor still cannot be used by anyone except of implementing classes

Because it prevents subclasses outside the package.

Yes, but why shall we prevent such subclasses? I am not against it, but just do not understand...

liach · 2018-01-02T23:19:44Z

gson/src/main/java/com/google/gson/reflect/impl/PreJava9ReflectionAccessor.java

+  /**
+   * {@inheritDoc}
+   */
+  public void makeAccessible(AccessibleObject ao) {


@Override annotation

liach · 2018-01-02T23:19:49Z

gson/src/main/java/com/google/gson/reflect/impl/UnsafeReflectionAccessor.java

+  /**
+   * {@inheritDoc}
+   */
+  public void makeAccessible(AccessibleObject ao) {


@Override annotation

inder123 · 2018-01-03T18:05:42Z

gson/src/main/java/com/google/gson/internal/reflect/ReflectionAccessor.java

+  }
+
+  private static ReflectionAccessor createReflectionAccessor() {
+    if (VersionUtils.getMajorJavaVersion() < 9) {


Could be inlined as
ReflectionAccessor instance = VersionUtils.getMajorJavaVersion() < 9 ? new PreJava9ReflectionAccessor() : new UnsafeReflectionAccessor();

inder123 · 2018-01-03T18:06:17Z

gson/src/main/java/com/google/gson/internal/reflect/UnsafeReflectionAccessor.java

+ */
+final class UnsafeReflectionAccessor extends ReflectionAccessor {
+
+  private static final Unsafe theUnsafe = getUnsafeInstance();


do these need to be static? Please make them non-static

I am not sure why you prefer instance fields over static ones, but sure, these may be change to non-static. What about static methods, like getUnsafeInstance()? If needed, they may be changed to non-static as well.

static fields will get initialized when class is loaded which is an unnecessary penalty in memory and loading time.

Static methods are fine (and slightly preferred by me).

inder123 · 2018-01-03T18:07:10Z

Thanks for patiently incorporating all the feedback

googlebot · 2018-01-03T18:52:43Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

swankjesse · 2018-01-03T20:02:15Z

I think this might be the wrong solution to the problem. Unfortunately, the right solution to this problem is a lot of work.

Instead of using a bigger, more powerful weapon to read & write platform types, I think Gson should have a new java-platform-typeadapters module.

Wherever Gson’s users are relying on reflection to read+write a platform type like java.util.UUID, we should handwrite a type adapter. That way we avoid getting into an arms race with the JDK maintainers who really don’t want their fields to be reflected upon.

swankjesse · 2018-01-03T20:03:28Z

... anyone who wants to use Gson with Java 9 to read+write platform types would need this module. And the Gson-using community would have to built the module to cover as many types as is reasonable.

JakeWharton · 2018-01-03T20:04:49Z

Strongly agree #1216 (comment). The module system is the opportunity to enforce something which never should have been allowed!

inder123 · 2018-01-03T21:09:14Z

@swankjesse Agree with you. We can start on adding some of these type adapters, and build upon it gradually.

* Java 9 support: use Unsafe-based reflection in Java 9+ fixes "illegal reflective access" warnings and exceptions * fix Codacy warnings * improve code quality based on PR review * improve code quality based on PR review * fix Codacy warning * improve code quality based on PR review * inlined createReflectionAccessor method

Revert google#1218 Usage of sun.misc.Unsafe to change internal AccessibleObject.override field to suppress JPMS warnings goes against the intentions of the JPMS and does not work anymore in newer versions, see google#1540. Therefore remove it and instead create a descriptive exception when making a member accessible fails. If necessary users can also still use `java` command line flags to open external modules.

…tor (#1902) * Remove UnsafeReflectionAccessor Revert #1218 Usage of sun.misc.Unsafe to change internal AccessibleObject.override field to suppress JPMS warnings goes against the intentions of the JPMS and does not work anymore in newer versions, see #1540. Therefore remove it and instead create a descriptive exception when making a member accessible fails. If necessary users can also still use `java` command line flags to open external modules. * Fix failing to serialize Collection or Map with inaccessible constructor Also remove tests which rely on Java implementation details. * Don't keep reference to access exception of ConstructorConstructor This also avoids a confusing stack trace, since the previously caught exception might have had a complete unrelated stack trace. * Remove Maven toolchain requirement * Address review feedback * Add back test for Security Manager

…tor (google#1902) * Remove UnsafeReflectionAccessor Revert google#1218 Usage of sun.misc.Unsafe to change internal AccessibleObject.override field to suppress JPMS warnings goes against the intentions of the JPMS and does not work anymore in newer versions, see google#1540. Therefore remove it and instead create a descriptive exception when making a member accessible fails. If necessary users can also still use `java` command line flags to open external modules. * Fix failing to serialize Collection or Map with inaccessible constructor Also remove tests which rely on Java implementation details. * Don't keep reference to access exception of ConstructorConstructor This also avoids a confusing stack trace, since the previously caught exception might have had a complete unrelated stack trace. * Remove Maven toolchain requirement * Address review feedback * Add back test for Security Manager

Java 9 support: use Unsafe-based reflection in Java 9+

2273f59

fixes "illegal reflective access" warnings and exceptions

googlebot added the cla: yes label Jan 1, 2018

fix Codacy warnings

9bbb504

inder123 reviewed Jan 2, 2018

View reviewed changes

improve code quality based on PR review

8f49b36

liach reviewed Jan 2, 2018

View reviewed changes

inder123 reviewed Jan 2, 2018

View reviewed changes

liach reviewed Jan 2, 2018

View reviewed changes

amogilev added 2 commits January 3, 2018 10:00

improve code quality based on PR review

6bea9e4

fix Codacy warning

504e1ce

inder123 reviewed Jan 3, 2018

View reviewed changes

amogilev and others added 2 commits January 4, 2018 01:32

improve code quality based on PR review

c0cde94

inlined createReflectionAccessor method

c74fa56

googlebot removed the cla: yes label Jan 3, 2018

googlebot added the cla: no label Jan 3, 2018

inder123 approved these changes Jan 3, 2018

View reviewed changes

inder123 merged commit 8445689 into google:master Jan 3, 2018

Marcono1234 mentioned this pull request Jun 10, 2020

com.google.gson.internal.reflect.UnsafeReflectionAccessor illegal reflective access operation on jdk12 #1540

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java 9 support: use Unsafe-based reflection in Java 9+ #1218

Java 9 support: use Unsafe-based reflection in Java 9+ #1218

amogilev commented Jan 1, 2018

inder123 Jan 2, 2018

inder123 Jan 2, 2018

inder123 Jan 2, 2018

inder123 Jan 2, 2018

inder123 Jan 2, 2018

inder123 Jan 2, 2018

inder123 Jan 2, 2018 •

edited

Loading

inder123 commented Jan 2, 2018

JakeWharton commented Jan 2, 2018

amogilev commented Jan 2, 2018

liach Jan 2, 2018

inder123 Jan 2, 2018

liach Jan 2, 2018

amogilev Jan 3, 2018

inder123 Jan 2, 2018

inder123 Jan 2, 2018

amogilev Jan 3, 2018

liach Jan 2, 2018

amogilev Jan 3, 2018

JakeWharton Jan 3, 2018

amogilev Jan 3, 2018

liach Jan 2, 2018

liach Jan 2, 2018

inder123 Jan 3, 2018

inder123 Jan 3, 2018

amogilev Jan 3, 2018

inder123 Jan 3, 2018

inder123 commented Jan 3, 2018

googlebot commented Jan 3, 2018

swankjesse commented Jan 3, 2018

swankjesse commented Jan 3, 2018

JakeWharton commented Jan 3, 2018

inder123 commented Jan 3, 2018

Java 9 support: use Unsafe-based reflection in Java 9+ #1218

Java 9 support: use Unsafe-based reflection in Java 9+ #1218

Conversation

amogilev commented Jan 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inder123 Jan 2, 2018 • edited Loading

Choose a reason for hiding this comment

inder123 commented Jan 2, 2018

JakeWharton commented Jan 2, 2018

amogilev commented Jan 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inder123 commented Jan 3, 2018

googlebot commented Jan 3, 2018

swankjesse commented Jan 3, 2018

swankjesse commented Jan 3, 2018

JakeWharton commented Jan 3, 2018

inder123 commented Jan 3, 2018

inder123 Jan 2, 2018 •

edited

Loading