Add `SetContextClassLoader` utility class #6575

basil · 2022-05-14T17:48:43Z

Promoting https://github.com/jenkinsci/ldap-plugin/blob/3731881c37d1c66f8f43101b7f8b0cdd5f1eba56/src/main/java/jenkins/security/plugins/ldap/SetContextClassLoader.java into a core utility class.

Example usages

Production code: jenkinsci/xcode-plugin#114
Test code: jenkinsci/jaxb-plugin#34

Proposed changelog entries

Introduced SetContextClassLoader utility class to assist in plugin class loading.

Proposed upgrade guidelines

N/A

Submitter checklist

(If applicable) Jira issue is well described
Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change) and are in the imperative mood. Examples
- Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
Appropriate autotests or explanation to why this change has no tests
New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, as appropriate.
For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

There are at least 2 approvals for the pull request and no outstanding requests for change
Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
Changelog entries in the PR title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood
Proper changelog labels are set so that the changelog can be generated automatically
If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

This reverts commit e8b04d1.

basil · 2022-05-14T18:21:47Z

core/src/main/java/jenkins/util/SetContextClassLoader.java

+    public SetContextClassLoader(Class<?> clazz) {
+        t = Thread.currentThread();
+        orig = t.getContextClassLoader();
+        // It is too bad that Reflection.getCallerClass() is a private API.
+        t.setContextClassLoader(clazz.getClassLoader());
+    }


Once we're on Java 11, we could perhaps remove the clazz argument and get the calling class via the StackWalker API.

FWIW I would much prefer the explicit Class argument as currently written here. It is hardly bothersome to pass, and sometimes you may actually want something other than the calling class: a class from a library, a plugin you depend upon, etc. Magically inferring an argument according to the caller is a poor practice in terms of legibility and predictability; for example, it means that supposedly safe refactorings like pull up method may quietly lead to different behavior.

Well you can have your opinion and I can have mine. I actually think a no-argument version is preferable since you can cut and paste code from one class to another without having to worry about updating the calling class argument to reflect the new home of the code.

basil · 2022-05-14T18:37:04Z

I think I am going leave this as draft/on-hold until we require Java 11. I would love to have a clean API from day 1 (with zero arguments), but that only seems possible with Java 11 features like StackWalker. I would rather avoid introducing multiple one-argument constructors only to have to deprecate them later.

jglick · 2022-05-19T13:54:46Z

core/src/main/java/jenkins/util/SetContextClassLoader.java

+    public SetContextClassLoader(Class<?> clazz) {
+        t = Thread.currentThread();
+        orig = t.getContextClassLoader();
+        // It is too bad that Reflection.getCallerClass() is a private API.
+        t.setContextClassLoader(clazz.getClassLoader());
+    }


FWIW I would much prefer the explicit Class argument as currently written here. It is hardly bothersome to pass, and sometimes you may actually want something other than the calling class: a class from a library, a plugin you depend upon, etc. Magically inferring an argument according to the caller is a poor practice in terms of legibility and predictability; for example, it means that supposedly safe refactorings like pull up method may quietly lead to different behavior.

core/src/main/java/jenkins/util/SetContextClassLoader.java

timja · 2022-08-01T10:30:15Z

core/src/main/java/jenkins/util/SetContextClassLoader.java

+     * @since TODO
+     */
+    public SetContextClassLoader() {
+        this(StackWalker.getInstance().getCallerClass());


cool can hopefully simplify a few APIs with this, e.g. https://github.com/jenkinsci/configuration-as-code-plugin/blob/e345fd8df353db1a9fbd930968b87fce30548438/test-harness/src/main/java/io/jenkins/plugins/casc/misc/Util.java#L172

jglick · 2022-08-01T14:13:05Z

core/src/main/java/jenkins/util/SetContextClassLoader.java

+    public SetContextClassLoader() {
+        this(StackWalker.getInstance().getCallerClass());


Still -0 on including the no-arg overload as in #6575 (comment): StackWalker and similar idioms should be avoided in production code.

(an Object argument delegating to the Class overload would be fine, for calls from non-static contexts)

basil · 2022-08-01T15:49:10Z

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process. Thanks!

jglick · 2022-08-01T16:02:10Z

BTW there are a few internal places that could use the new utility as a demonstration: ClassicPluginStrategy, ContextResettingExecutorService, TimerTest, PluginManagerTest

basil · 2022-08-01T16:25:19Z

Two examples of usages of the new utility were provided in the PR description as a demonstration. There are a lot more that could be done.

Add SetContextClassLoader utility class

a7a66a0

basil added the developer Changes which impact plugin developers label May 14, 2022

basil added 2 commits May 14, 2022 10:59

Clarify Javadoc

ee5cf6e

Add at-since

80c7b29

basil added the squash-merge-me Unclean or useless commit history, should be merged only with squash-merge label May 14, 2022

basil added 4 commits May 14, 2022 11:05

Remove references to UberClassLoader

e8b04d1

Revert "Remove references to UberClassLoader"

22f9c02

This reverts commit e8b04d1.

Be more flexible

752dc64

Express yearning for a public API

d531df7

basil commented May 14, 2022

View reviewed changes

basil added the on-hold This pull request depends on another event/release, and it cannot be merged right now label May 14, 2022

basil marked this pull request as draft May 14, 2022 18:37

basil closed this May 16, 2022

jglick reviewed May 19, 2022

View reviewed changes

basil added 2 commits July 30, 2022 17:52

Merge remote-tracking branch 'origin/master' into SetContextClassLoader

b13e3a8

improvements

8d38b48

basil reopened this Jul 31, 2022

basil removed the on-hold This pull request depends on another event/release, and it cannot be merged right now label Jul 31, 2022

basil marked this pull request as ready for review July 31, 2022 01:00

basil requested a review from jglick July 31, 2022 01:17

timja approved these changes Aug 1, 2022

View reviewed changes

jglick reviewed Aug 1, 2022

View reviewed changes

daniel-beck approved these changes Aug 1, 2022

View reviewed changes

basil added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Aug 1, 2022

basil merged commit f77a28b into jenkinsci:master Aug 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `SetContextClassLoader` utility class #6575

Add `SetContextClassLoader` utility class #6575

basil commented May 14, 2022 •

edited by gitpod-io bot

Loading

basil May 14, 2022

jglick May 19, 2022

basil Jul 31, 2022

basil commented May 14, 2022

jglick May 19, 2022

timja Aug 1, 2022

jglick Aug 1, 2022

jglick Aug 1, 2022

basil commented Aug 1, 2022

jglick commented Aug 1, 2022

basil commented Aug 1, 2022

		public SetContextClassLoader() {
		this(StackWalker.getInstance().getCallerClass());

Add SetContextClassLoader utility class #6575

Add SetContextClassLoader utility class #6575

Conversation

basil commented May 14, 2022 • edited by gitpod-io bot Loading

Example usages

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

basil May 14, 2022

Choose a reason for hiding this comment

jglick May 19, 2022

Choose a reason for hiding this comment

basil Jul 31, 2022

Choose a reason for hiding this comment

basil commented May 14, 2022

jglick May 19, 2022

Choose a reason for hiding this comment

timja Aug 1, 2022

Choose a reason for hiding this comment

jglick Aug 1, 2022

Choose a reason for hiding this comment

jglick Aug 1, 2022

Choose a reason for hiding this comment

basil commented Aug 1, 2022

jglick commented Aug 1, 2022

basil commented Aug 1, 2022

Add `SetContextClassLoader` utility class #6575

Add `SetContextClassLoader` utility class #6575

basil commented May 14, 2022 •

edited by gitpod-io bot

Loading