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 Activation erroneously assumes that classes can be loaded from Thread#getContextClassLoader #145

Merged
merged 6 commits into from
Feb 14, 2024

Conversation

jmehrens
Copy link
Contributor

@jmehrens jmehrens commented Jan 8, 2024

#144

Signed-off-by: jmehrens jason_mehrens@hotmail.com

…m Thread#getContextClassLoader

Signed-off-by: jmehrens <jason_mehrens@hotmail.com>
@jmehrens jmehrens self-assigned this Jan 9, 2024
Signed-off-by: jmehrens <jason_mehrens@hotmail.com>
…m Thread#getContextClassLoader jakartaee#144

Signed-off-by: jmehrens <jason_mehrens@hotmail.com>
Copy link
Contributor

@lukasj lukasj left a comment

Choose a reason for hiding this comment

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

update year in copyright headers of modified files, please (ie 2021 -> 2021, 2024)

…m Thread#getContextClassLoader jakartaee#145

Signed-off-by: jmehrens <jason_mehrens@hotmail.com>
…m Thread#getContextClassLoader jakartaee#145

Signed-off-by: jmehrens <jason_mehrens@hotmail.com>
@jmehrens jmehrens requested a review from lukasj February 14, 2024 05:22
…m Thread#getContextClassLoader jakartaee#145

Signed-off-by: jmehrens <jason_mehrens@hotmail.com>
@lukasj lukasj merged commit 4b8285b into jakartaee:master Feb 14, 2024
4 checks passed
lukasj pushed a commit to lukasj/jaf-api that referenced this pull request Feb 15, 2024
…m Thread#getContextClassLoader (jakartaee#145)

Signed-off-by: jmehrens <jason_mehrens@hotmail.com>
(cherry picked from commit 4b8285b)
lukasj pushed a commit that referenced this pull request Feb 15, 2024
…m Thread#getContextClassLoader (#145)

Signed-off-by: jmehrens <jason_mehrens@hotmail.com>
(cherry picked from commit 4b8285b)
@jmehrens
Copy link
Contributor Author

@basil I ported the JakartaMail classloading changes to JakartaActivation. Your existing jenkins activation workaround should continue function as normally. However, the changes should allow you to migrate away from the using the workaround if you so desire.

If you get some free cycles test and let me know. As always, if you see an issue let us know.

@basil
Copy link

basil commented Mar 6, 2024

If you get some free cycles test and let me know. As always, if you see an issue let us know.

@jmehrens Thank you very much for working on this. With the release of Jakarta Mail 2.1.3, I have finally been able to upgrade the Jenkins ecosystem to Jakarta Mail 2.1.3, Jakarta Activation 2.1.3, and Eclipse Angus. The change has been released to production users since Monday with no reported issues. Thank you very much!

I ported the Jakarta Mail classloading changes to Jakarta Activation. Your existing jenkins activation workaround should continue function as normally. However, the changes should allow you to migrate away from the using the workaround if you so desire.

I tried removing my workaround, and although your changes worked correctly, I still needed the workaround for an unexpected reason. You might notice my workaround is in the Jenkins Jakarta Mail plugin, which can see classes from the Jenkins Jakarta Activation plugin (but not vice versa). When I removed the workaround, your new code correctly kicked in and used the calling class loader from Jenkins Jakarta Activation plugin, but since that class loader could not see the Jenkins Jakarta Mail plugin, the mailcap was empty.

Since we don't allow circular dependencies between Jenkins plugins, the only way for me to fix this would be to combine the Jenkins Jakarta Activation plugin and Jenkins Jakarta Mail plugin into a single plugin. I might do that eventually, in which case I am confident your change would allow me to remove the workaround. But for now, this is a huge step forward, so I am going to declare success in the short to medium term and save that problem for another day.

@jmehrens
Copy link
Contributor Author

jmehrens commented Mar 7, 2024

@basil That is good news! Thanks for testing and providing feedback.

When I removed the workaround, your new code correctly kicked in and used the calling class loader from Jenkins Jakarta Activation plugin, but since that class loader could not see the Jenkins Jakarta Mail plugin, the mailcap was empty.

Interesting. I'll do some more thinking on this. Presumably, resource lookup would work if Jakarta Mail classloader was given to Jakarta Activation. Next release it would be possible to use stackwalker to locate calling classloader. I'll do some thinking on this.

@basil
Copy link

basil commented Mar 7, 2024

Presumably, resource lookup would work if Jakarta Mail classloader was given to Jakarta Activation.

Indeed, and that's effectively what my workaround is doing by using a classloader from Jenkins Jakarta Mail plugin (rather than Jenkins Jakarta Activation plugin), albeit by setting the thread's context class loader. That's also why I'm sure this problem would also be fixed if I combined the two Jenkins plugins into a single Jenkins plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jakarta Activation erroneously assumes that classes can be loaded from Thread#getContextClassLoader
3 participants