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

Add GsonBuilder.disableJdkUnsafe() #1904

Merged
merged 2 commits into from Dec 30, 2021

Conversation

Marcono1234
Copy link
Collaborator

@Marcono1234 Marcono1234 commented Jun 4, 2021

Resolves #445
Resolves partially #1387
Resolves partially #1695; should still be kept open because usage of Unsafe should be better documented

No test is added to verify that by default Unsafe is used because it appears 38 tests already depend on this behavior and would fail that was not the case.

Note that for Android this actually disables more than just sun.misc.Unsafe usage, because there Gson also tries internal ObjectStreamClass and ObjectInputStream methods, but I don't think this is worth mentioning.

@google-cla google-cla bot added the cla: yes label Jun 4, 2021
@Marcono1234 Marcono1234 force-pushed the marcono1234/disable-unsafe branch 2 times, most recently from 21d0f85 to 2b70a54 Compare December 29, 2021 23:19
@Marcono1234
Copy link
Collaborator Author

Maybe in the future it should also be considered to make usage of Unsafe opt-in. It now and then causes confusing issues for users where fields are not initialized (because that is part of the constructor, which Unsafe does not execute).

Copy link
Member

@eamonnmcmanus eamonnmcmanus left a comment

Choose a reason for hiding this comment

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

Thanks, this is great! I think using Unsafe.allocateInstance is kind of horrifying, and it certainly shouldn't be the default behaviour. Unfortunately, it is, and changing that would probably break too many things. Making it at least possible to avoid Unsafe is probably the best we can do.

* runtimes. For example Android does not provide {@code Unsafe}, or only with limited
* functionality. Additionally {@code Unsafe} creates instances without executing any
* constructor or initializer block, or performing initialization of field values. This can
* lead to surprising and difficult to debug errors.<br>
Copy link
Member

Choose a reason for hiding this comment

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

Could you start a new paragraph on the next line rather than <br>? Or just leave out <br>.

if (useJdkUnsafe) {
return new ObjectConstructor<T>() {
private final UnsafeAllocator unsafeAllocator = UnsafeAllocator.create();
@SuppressWarnings("unchecked")
Copy link
Member

Choose a reason for hiding this comment

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

While you are here, could you reduce the scope of this?

@Override public T construct() {
  ...
    @SuppressWarnings("unchecked")
    T newInstance = (T) unsafeAllocator.newInstance(rawType);
    return newInstance;
  ...


public void testDisableJdkUnsafe() {
Gson gson = new GsonBuilder()
.disableJdkUnsafe()
Copy link
Member

Choose a reason for hiding this comment

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

Very nitpicky comment here, but these continuation lines should be indented +4 according to Google Java style.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No worries, such comments are fine

@eamonnmcmanus eamonnmcmanus merged commit 615c883 into google:master Dec 30, 2021
@Marcono1234 Marcono1234 deleted the marcono1234/disable-unsafe branch December 30, 2021 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GsonBuilder.disableUnsafe
2 participants