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

[JENKINS-39159] If we create a URLClassLoader we should also close it #105

Merged
merged 2 commits into from Feb 27, 2017

Conversation

Projects
None yet
4 participants
@jglick
Copy link
Member

commented Feb 6, 2017

@@ -3,7 +3,8 @@
<parent>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plugin</artifactId>
<version>2.6</version>
<version>2.21</version>

This comment has been minimized.

Copy link
@svanoort

svanoort Feb 10, 2017

Member

Needed?

This comment has been minimized.

Copy link
@jglick

jglick Feb 10, 2017

Author Member

I update these as a matter of course.

@svanoort
Copy link
Member

left a comment

I hope I'm misunderstanding something here but I think this will not work right (🐛).

My understanding is that the finally method will be invoked before the return. Which is fine and dandy... EXCEPT that what happens if we have a child classloader (returned) that depends on this now-closed parent?

@jglick

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2017

what happens if we have a child classloader (returned) that depends on this now-closed parent?

Then any attempts to perform fresh class (or resource) loads from that child class loader will return errors.

Now consider what would need to happen. First, note that there is an implicit declared type for the return value: whatever the calling code casts it to, defined in the Java Platform or some Jenkins (core or plugin) code. Often this is an effectively final type like Boolean or String, or simply Void since the return value is ignored (the script may only call mutators). Suppose it is a type which the user script is intended to implement (not merely abstractMap which would typically be a HashMap). This is largely hypothetical; the only example I can come up with is that an Object is returned and toString is called (implicitly).

Now suppose that the user specifies a classpath for their script, which is generally discouraged (this whole feature should be deprecated IMO). And the return value is of a type defined in that external JAR.

Merely returning a class defined in that JAR is fine—it will be loaded, evaluate will finish, and toString is called. So suppose in their /home/jenkins/mystuff.jar they had mystuff/A.class

package mystuff;
public class A {
    @Override
    public String toString() {
        return B.print();
    }
}

and mystuff/B.class

package mystuff;
class B {
    public static String print() {
        return "OK!";
    }
}

and the user script is

new mystuff.A()

then the instance of A will be returned, and depending on the class loading behavior of the JVM, B might or might not yet have been resolved, so when the caller prints the return value you will get an error.

Compare this quite contrived problem to the very real FileNotFoundException reported by a user and apparently caused by failing to close a file handle.

@reviewbybees

This comment has been minimized.

Copy link

commented Feb 25, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@svanoort

This comment has been minimized.

Copy link
Member

commented Feb 27, 2017

@jglick Okay that makes sense, but let me narrow down the question: is there any circumstance besides a custom groovy classpath in which the GroovyShell would invoke the UrlClassLoader after the return here? Like anything that would invoke it for classloading within the shell.

If yes, then this is not safe and we need to create a cleanup thread that closes the classloader when we're done with the shell (or delegate to some cleanup/after-run hook in Groovy itself -- I do not know if there is such an animal or not).

If no then this is quite a successful solution.

@jglick

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2017

is there any circumstance besides a custom groovy classpath in which the GroovyShell would invoke the UrlClassLoader

We only create the URLClassLoader to begin with when you are using a custom Groovy classpath.

in which the GroovyShell would invoke the UrlClassLoader after the return here

In the (hopefully contrived) case above, the URLClassLoader would be used after the return from evaluate, as I mentioned. GroovyShell is not involved in that case. The class load is done by the JVM’s (somewhat) lazy class linking mechanism.

when we're done with the shell

Unless the return value is of a type defined in the script itself, the GroovyShell should not be reachable once evaluate returns. Even if it is, and there is a custom classpath, it is unlikely the URLClassLoader would be needed after return. Since the Groovy compiler will resolve any type mentioned from the script, you would need another convoluted case to see such a problem:

package mystuff;
public class A {
    public String whatever() {
        return B.something();
    }
}
package mystuff;
public class B {
    public static String something() {
        return "late load";
    }
}
class X {
    @Field a = new A()
    @Override def toString() {a.whatever()}
}
new X()
@svanoort
Copy link
Member

left a comment

@jglick Thanks, that helps clarify. I'm not hearing complete confidence that this change will not introduce further issues -- if there was a reasonable way to test this I'd request this before approval... but I'm not sure we could actually cover it.

🐝 because we're replacing a known issue with a possible but unlikely issue. I trust you to be able to cut a superfast fix or revert if that theoretical issue becomes a real one.

@jglick

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2017

we're replacing a known issue with a possible but unlikely issue

Right. This is the tricky bit with URLClassLoader.close: garbage collection is too haphazard to be relied on for an externally visible effect (file handle closure), yet lazy class linking makes it awkward to prove that all activity has ceased with some object. (For example this would be an obstacle to implementing dynamic plugin disable in Jenkins: some timer task involving novel plugin code could still be scheduled.)

@jglick jglick merged commit d6bddd0 into jenkinsci:master Feb 27, 2017

1 check passed

Jenkins This pull request looks good
Details

@jglick jglick deleted the jglick:URLClassLoader.close-JENKINS-39159 branch Feb 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.