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

Jakarta Mail erroneously assumes that classes can be loaded from Thread#getContextClassLoader #701

Merged
merged 14 commits into from
Jan 5, 2024

Conversation

jmehrens
Copy link
Contributor

@jmehrens jmehrens marked this pull request as draft November 12, 2023 05:15
@jmehrens jmehrens requested review from jbescos and lukasj November 12, 2023 05:15
@jmehrens
Copy link
Contributor Author

@lukasj @jbescos Here is my attempt at fixing this error. Basically this change probes various classloaders in order of preference to find the provider that is usable in the caller classloader. Let me know what you think. For sure it seems suspect to me that I'm not using a PrivilegedAction to fetch the classloader. Thoughts? Testing this change is not going to be easy.

Copy link
Member

@jbescos jbescos left a comment

Choose a reason for hiding this comment

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

LGTM

Probe for HK2 targets in different loaders.
Signed-off-by: jmehrens <[email protected]>
Signed-off-by: jmehrens <[email protected]>
Signed-off-by: jmehrens <[email protected]>
Signed-off-by: jmehrens <[email protected]>
Signed-off-by: jmehrens <[email protected]>
Signed-off-by: jmehrens <[email protected]>
Signed-off-by: jmehrens <[email protected]>
@jmehrens jmehrens requested a review from jbescos November 19, 2023 06:21
@jmehrens
Copy link
Contributor Author

jmehrens commented Nov 19, 2023

@jbescos Fixed issues with:

  • privileged actions when needed.
  • Unify API so null classloader means use system classloader matching Service API.
  • Added a simple test case.
  • Found a bug where hard coded implementation classes were the wrong package name.

@jmehrens jmehrens marked this pull request as ready for review November 21, 2023 20:01
Copy link

@basil basil left a comment

Choose a reason for hiding this comment

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

I have reproduced the issue interactively in Jenkins with the Gmail SMTP server at HEAD with commit 6c9ac50 and verified that the issue is resolved with this PR.

@basil
Copy link

basil commented Jan 4, 2024

A merge and release of this PR would unblock our upgrade to recent versions of Mail API and allow us to resolve issues like JENKINS-70627.

@jmehrens
Copy link
Contributor Author

jmehrens commented Jan 4, 2024

I have reproduced the issue interactively in Jenkins with the Gmail SMTP server at HEAD with commit 6c9ac50 and verified that the issue is resolved with this PR.

Thanks @basil for testing. I'm surprised this worked as is with out a similar fix to JAF. Either way I'll try to get this merged.

@basil
Copy link

basil commented Jan 4, 2024

I'm surprised this worked as is with out a jakartaee/jaf-api#144. Either way I'll try to get this merged.

I have been working around the problem in Jakarta Activation with https://github.com/jenkinsci/jakarta-mail-api-plugin/tree/5a95bb71b308c7ba2dab277deffb36dfe1e03c8c/src/main/java/io/jenkins/plugins/jakarta/activation for years now. But I have no workaround for this issue in Jakarta Mail.

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.

Jakarta Mail erroneously assumes that classes can be loaded from Thread#getContextClassLoader
4 participants