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

Use application classloader for proxies #419

Merged
merged 1 commit into from
Oct 14, 2020

Conversation

tevans78
Copy link
Contributor

Signed-off-by: tevans tevans@uk.ibm.com

fixes #418

@Emily-Jiang
Copy link
Contributor

Thanks @tevans78 ! @radcortez can you take a look to see whether you have any comments?

@radcortez
Copy link
Member

Thank you. Please, open this up to master and I'll rebase. Additional specific changes for the branch can be added later.

}
}

private static Class<?> loadClass(final String className, final byte[] classBytes) {
Copy link
Member

Choose a reason for hiding this comment

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

Please, keep a separate call to this method, so it easier to substitute in GraalVM: https://github.com/quarkusio/quarkus/blob/ff457754943c36a620102571f854432deb41dbf7/core/runtime/src/main/java/io/quarkus/runtime/configuration/Substitutions.java#L60

We need to remove any references to MethodHandles for native compilation. We could substitute the method loadClass(final Class<?> parent, final String className, final byte[] classBytes), but that means that we need to keep the body in sync between the implementation and substitution, since we still want to call loadClass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I needed to pass parent through to the defineClass call, the parameter lists become the same. So to keep the methods separate, one of the methods needs to change name. Any suggestions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

defineClass instead of the second loadClass method?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, just call it defineClass.

Copy link
Contributor

Choose a reason for hiding this comment

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

I made this change when I did the rebase on master

@Emily-Jiang
Copy link
Contributor

@radcortez do you mean all PRs need to be opened against master and you will rebase the mpconfig20 against master? Just to be clear this change is needed for MP Config 2.0 support for Open Liberty.

@tevans78 tevans78 requested a review from a team as a code owner October 14, 2020 11:07
Copy link
Member

@radcortez radcortez left a comment

Choose a reason for hiding this comment

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

Hi @tevans78,

Can you please rebase your commit just with master? We can add the specific code change related with ConfigMappingClass after we merge and rebase.

Signed-off-by: tevans <tevans@uk.ibm.com>
@tevans78
Copy link
Contributor Author

@radcortez sure, I haven't had a chance to do anything with this PR yet today ... I have no idea why github thinks I requested a review 2 hours ago?!

@tevans78 tevans78 changed the base branch from mpconfig20 to master October 14, 2020 13:18
@radcortez
Copy link
Member

Oh... I thought you did something to it, because I got the review notification, plus the PR had some weird history.

@tevans78
Copy link
Contributor Author

@radcortez ready for re-review ;-) many thanks to @Azquelt for doing the rebase for me.

@radcortez
Copy link
Member

Thank you @tevans78 and @Azquelt!

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.

ConfigMapping "Proxy" classes need to be created using the application's classloader
4 participants