Skip to content

Conversation

eamonnmcmanus
Copy link
Member

Fixes #2739.

@Marcono1234
Copy link
Contributor

Marcono1234 commented Sep 13, 2024

Thanks a lot for noticing and troubleshooting this! It was also my fault for not paying attention to this during the refactoring.

In addition to your changes here, we could probably make it even safer by removing the JsonElementTypeAdapter.FACTORY field and instead using the newTypeHierarchyFactory call directly as initializer for TypeAdapters.JSON_ELEMENT_FACTORY, e.g.:

public static final TypeAdapterFactory JSON_ELEMENT_FACTORY =
    newTypeHierarchyFactory(JsonElement.class, JSON_ELEMENT);

That would then completely remove the direct cyclic dependency between JsonElementTypeAdapter and TypeAdapters.

(Also, at least in my opinion, we could also un-deprecate the fields in TypeAdapters again and remove the Javadoc. I think it was only useful in the situation where those fields were not used internally by Gson anymore.)

The idea was to discourage people from using these, but they have been there for
a long time and the deprecation isn't going to have much effect. Meanwhile the
deprecation required `@SuppressWarnings`, which was annoying.

Also move `JsonElementTypeFactory.ADAPTER` back into `TypeAdapters` so there is
no further circular dependency between the two classes. I think it was harmless
but it's easy enough to avoid.
@eamonnmcmanus
Copy link
Member Author

Thanks @Marcono1234, those changes seem reasonable.

@Deprecated
public static final TypeAdapterFactory JSON_ELEMENT_FACTORY = JsonElementTypeAdapter.FACTORY;
public static final TypeAdapterFactory JSON_ELEMENT_FACTORY =
TypeAdapters.newTypeHierarchyFactory(JsonElement.class, JSON_ELEMENT);
Copy link
Contributor

Choose a reason for hiding this comment

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

Really minor: You can omit the TypeAdapters. qualifier here (unless you added it intentionally) since this is already within TypeAdapters.

Suggested change
TypeAdapters.newTypeHierarchyFactory(JsonElement.class, JSON_ELEMENT);
newTypeHierarchyFactory(JsonElement.class, JSON_ELEMENT);

Copy link
Member

@cpovirk cpovirk left a comment

Choose a reason for hiding this comment

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

Sorry, this wasn't actually that complicated, but I needed to find a minute when I wasn't half-thinking about something else at the time.

@eamonnmcmanus eamonnmcmanus merged commit 6160932 into google:main Sep 16, 2024
11 checks passed
@eamonnmcmanus eamonnmcmanus deleted the classloadingdeadlock branch September 16, 2024 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Class loading deadlock with JsonElementTypeAdapter

3 participants