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

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

Merged
merged 7 commits into from
Jan 3, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,15 @@

import com.google.gson.InstanceCreator;
import com.google.gson.JsonIOException;
import com.google.gson.reflect.ReflectionAccessor;
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 ReflectionAccessor accessor = ReflectionAccessor.getInstance();

public ConstructorConstructor(Map<Type, InstanceCreator<?>> instanceCreators) {
this.instanceCreators = instanceCreators;
Expand Down Expand Up @@ -98,7 +100,7 @@ private <T> ObjectConstructor<T> newDefaultConstructor(Class<? super T> rawType)
try {
final Constructor<? super T> constructor = rawType.getDeclaredConstructor();
if (!constructor.isAccessible()) {
constructor.setAccessible(true);
accessor.makeAccessible(constructor);
}
return new ObjectConstructor<T>() {
@SuppressWarnings("unchecked") // T is the same raw type as is requested
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import com.google.gson.internal.Excluder;
import com.google.gson.internal.ObjectConstructor;
import com.google.gson.internal.Primitives;
import com.google.gson.reflect.ReflectionAccessor;
import com.google.gson.reflect.TypeToken;
import com.google.gson.stream.JsonReader;
import com.google.gson.stream.JsonToken;
Expand All @@ -49,6 +50,7 @@ public final class ReflectiveTypeAdapterFactory implements TypeAdapterFactory {
private final FieldNamingStrategy fieldNamingPolicy;
private final Excluder excluder;
private final JsonAdapterAnnotationTypeAdapterFactory jsonAdapterFactory;
private final ReflectionAccessor accessor = ReflectionAccessor.getInstance();

public ReflectiveTypeAdapterFactory(ConstructorConstructor constructorConstructor,
FieldNamingStrategy fieldNamingPolicy, Excluder excluder,
Expand Down Expand Up @@ -154,7 +156,7 @@ private Map<String, BoundField> getBoundFields(Gson context, TypeToken<?> type,
if (!serialize && !deserialize) {
continue;
}
field.setAccessible(true);
accessor.makeAccessible(field);
Type fieldType = $Gson$Types.resolve(type.getType(), raw, field.getGenericType());
List<String> fieldNames = getFieldNames(field);
BoundField previous = null;
Expand Down
66 changes: 66 additions & 0 deletions gson/src/main/java/com/google/gson/reflect/ReflectionAccessor.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
* Copyright (C) 2017 The Gson authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.google.gson.reflect;

import com.google.gson.reflect.impl.PreJava9ReflectionAccessor;
import com.google.gson.reflect.impl.UnsafeReflectionAccessor;
import com.google.gson.util.VersionUtils;

import java.lang.reflect.AccessibleObject;

/**
* Provides a replacement for {@link AccessibleObject#setAccessible(boolean)}, which may be used to
* avoid reflective access issues appeared in Java 9, like {@link java.lang.reflect.InaccessibleObjectException}
* thrown or warnings like
* <pre>
* WARNING: An illegal reflective access operation has occurred
* WARNING: Illegal reflective access by ...
* </pre>
* <p/>
* Works both for Java 9 and earlier Java versions.
*/
public abstract class ReflectionAccessor {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package-private constructor please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it prevents subclasses outside the package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* Does the same as {@code ao.setAccessible(true)}, but never throws
* {@link java.lang.reflect.InaccessibleObjectException}
*/
public abstract void makeAccessible(AccessibleObject ao);

/**
* Obtains a {@link ReflectionAccessor} instance suitable for the current Java version.
* <p>
* You may need one a reflective operation in your code throws {@link java.lang.reflect.InaccessibleObjectException}.
* In such a case, use {@link ReflectionAccessor#makeAccessible(AccessibleObject)} on a field, method or constructor
* (instead of basic {@link AccessibleObject#setAccessible(boolean)}).
*/
public static ReflectionAccessor getInstance() {
return ReflectionAccessorHolder.instance;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

private static ReflectionAccessor createReflectionAccessor() {
if (VersionUtils.getMajorJavaVersion() < 9) {
return new PreJava9ReflectionAccessor();
} else {
return new UnsafeReflectionAccessor();
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* Copyright (C) 2017 The Gson authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.google.gson.reflect.impl;

import com.google.gson.reflect.ReflectionAccessor;

import java.lang.reflect.AccessibleObject;

/**
* A basic implementation of {@link ReflectionAccessor} which is suitable for Java 8 and below.
* <p>
* This implementation just calls {@link AccessibleObject#setAccessible(boolean) setAccessible(true)}, which worked
* fine before Java 9.
*/
public final class PreJava9ReflectionAccessor extends ReflectionAccessor {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class can be package private


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Override annotation

ao.setAccessible(true);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
* Copyright (C) 2017 The Gson authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.google.gson.reflect.impl;

import com.google.gson.reflect.ReflectionAccessor;
import sun.misc.Unsafe;

import java.lang.reflect.AccessibleObject;
import java.lang.reflect.Field;

/**
* An implementation of {@link ReflectionAccessor} based on {@link Unsafe}.
* <p>
* 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 final class UnsafeReflectionAccessor extends ReflectionAccessor {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package private?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...


private static final Unsafe theUnsafe = getUnsafeInstance();
Copy link
Collaborator

@inder123 inder123 Jan 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

private static final Field overrideField = getOverrideField();

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Override annotation

if (theUnsafe != null && overrideField != null) {
long overrideOffset = theUnsafe.objectFieldOffset(overrideField);
theUnsafe.putBoolean(ao, overrideOffset, true);
}
}

private static Unsafe getUnsafeInstance() {
try {
Field unsafeField = Unsafe.class.getDeclaredField("theUnsafe");
unsafeField.setAccessible(true);
return (Unsafe) unsafeField.get(null);
} catch (Exception e) {
e.printStackTrace();
return null;
}
}

private static Field getOverrideField() {
try {
return AccessibleObject.class.getDeclaredField("override");
} catch (NoSuchFieldException e) {
e.printStackTrace();
return null;
}
}
}
3 changes: 2 additions & 1 deletion gson/src/main/java/com/google/gson/reflect/package-info.java
Original file line number Diff line number Diff line change
@@ -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.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two spaces between as and some

*
* @author Inderjeet Singh, Joel Leitch
*/
Expand Down