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

ProtoAdapter and FieldBinding should use rawType's classloader. #2388

Merged
merged 5 commits into from
Feb 28, 2023

Conversation

autonomousapps
Copy link
Contributor

@autonomousapps autonomousapps commented Jan 29, 2023

It is arguably a bug that ProtoAdapter implicitly uses the classLoader from itself when calling Class.forName(String). Instead, we should use Class.forName(String, boolean, ClassLoader), and the ClassLoader argument should be supplied by rawType.

Original description

This draft PR is intended first for discussion purposes, and to get feedback on what it would take to land something like this on trunk. We need this in our project because the wire-runtime library is loaded in a different classloader than the main app classes, leading to serialization errors when wire tries to instantiate a class instance via Class.forName(String). Arguably, best practice in cases like this indicates that an overload ought to be provided such that a library can use Class.forName(String, boolean, ClassLoader), because in general libraries cannot guarantee they'll be loaded in the same classloaders as application classes.

Verified

This commit was signed with the committer’s verified signature.
Byron Sebastian Thiel
@autonomousapps
Copy link
Contributor Author

@oldergod @bendotli I've updated this PR to use default arguments everywhere (I think), and also to use Optional<ClassLoader> = Optional.empty() in two places to handle the fact that a null classloader has a special value on the JVM. Let me know what you think.

@autonomousapps
Copy link
Contributor Author

autonomousapps commented Feb 10, 2023

With a test failure like the following, is it literally saying that it doesn't like that I made the declaration take multiple lines?

 public final class Pizza extends Message<Pizza, Pizza.Builder> {
-  public static final ProtoAdapter<Pizza> ADAPTER = ProtoAdapter.newMessageAdapter(
-    Pizza.class,
-    "type.googleapis.com/squareup.proto3.Pizza",
-    Syntax.PROTO_3,
-    Pizza.class.getClassLoader()
-  );
+  public static final ProtoAdapter<Pizza> ADAPTER = ProtoAdapter.newMessageAdapter(Pizza.class, "type.googleapis.com/squareup.proto3.Pizza", Syntax.PROTO_3);

@oldergod
Copy link
Member

@autonomousapps for the build fail, it says "Test files changed. Did you run ./gradlew generateTests?"

@autonomousapps
Copy link
Contributor Author

for the build fail, it says "Test files changed. Did you run ./gradlew generateTests?"

a-ha, I did not! Thanks for the tip.

@autonomousapps
Copy link
Contributor Author

Fixed the tests (./gradlew test succeeds), removed the injectable classLoader (we only need rawType's classLoader), and also changed all parameter names from loader to classLoader. @oldergod @swankjesse

@autonomousapps autonomousapps marked this pull request as ready for review February 28, 2023 00:13
@autonomousapps autonomousapps changed the title ProtoAdapter and FieldBinding need injectable classloader. ProtoAdapter and FieldBinding should use rawType's classloader. Feb 28, 2023
Copy link
Member

@oldergod oldergod left a comment

Choose a reason for hiding this comment

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

Let's go

@oldergod oldergod merged commit aa14ea3 into square:master Feb 28, 2023
@autonomousapps autonomousapps deleted the trobalik.classloader branch March 1, 2023 20:23
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.

None yet

4 participants