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.

Sign up for GitHub

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

Jump to bottom

Force class load on UserRequest, see JENKINS-19445 #82

Merged
merged 3 commits into from
May 9, 2016
Merged

Force class load on UserRequest, see JENKINS-19445 #82

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5d0c647
Force class load on UserRequest, see JENKINS-19445
jenkins-bbva Apr 14, 2016
9177198
Update UserRequest as per code review comments
jenkins-bbva Apr 20, 2016
8b4966c
Fix Logger.log() invocation
jenkins-bbva Apr 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/main/java/hudson/remoting/UserRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,32 @@ public UserRequest(Channel local, Callable<?,EXC> c) throws IOException {
return result;
}

static boolean workaroundDone = false;
Copy link
Member

Choose a reason for hiding this comment

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

should be private

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, many time away from coding in Java.

protected UserResponse<RSP,EXC> perform(Channel channel) throws EXC {
try {
ClassLoader cl = channel.importedClassLoaders.get(classLoaderProxy);

// Allow forcibly load of a class, allows to workaround:
// @See https://issues.jenkins-ci.org/browse/JENKINS-19445
// @Related https://issues.tmatesoft.com/issue/SGT-451
final String clazz = System.getProperty(RemoteClassLoader.class.getName() + ".force", null);
Copy link
Member

Choose a reason for hiding this comment

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

I would vote for initializing it once as a static variable.
I don't see how the dynamic provisioning may help, but it decreases the performance a bit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only point we are certain to have the same ClassLoader with the same rules. Not sure how to obtain the same ClassLoader from a static block. We checked all the flow from Launcher to here, as the only point were it would be safe, and early enough to prevent the blockage.

if ( clazz != null && !workaroundDone) {
// java.lang classes can only be instantiated by the bootstrap Classloader.
// Guarantees that *all* threads with whatever Classloader in use, have the
// same mutex instance: an intance of java.lang.Class<java.lang.Object>
synchronized(java.lang.Object.class)
{
workaroundDone = true;
try {
final Class<?> loaded = Class.forName( clazz, true, cl );
System.err.println("Loaded class: '" + clazz + "' using classloader: " + cl);
Copy link
Member

Choose a reason for hiding this comment

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

  1. Should be java.util.logging.Logger instead of the raw output
  2. It would be also great to move such IO operation outside the Object class lock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Which logger name?
  2. Good idea

Copy link
Member

Choose a reason for hiding this comment

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

In the Jenkins project we commonly use class name as a logger name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we are using RemoteClassLoader for the property, I used the same class for logging.

Copy link
Member

Choose a reason for hiding this comment

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

OK

} catch (final ClassNotFoundException cnfe) {
// not big deal, print and swallow exception
System.err.println("Error finding class '" + clazz + "' using classloader: " + cl);
}
}
}

RSP r = null;
Channel oldc = Channel.setCurrent(channel);
try {
Expand Down