basil commented on pull request #149: URL: https://github.com/apache/ant/pull/149#issuecomment-884318912
Thanks for the quick reply @jaikiran! The bug you linked to is very helpful. I am pleased to report that the original script given by Kohsuke in [this comment](https://issues.jenkins.io/browse/JENKINS-22310?focusedCommentId=197405&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-197405) still reproduces the problem in Jenkins core when the `super(parent)` calls are removed. So I have a minimal reproducible example that can be used to validate different approaches. I now see that Kohsuke's approach was Solution 1 described in [Bug 35436](https://bz.apache.org/bugzilla/show_bug.cgi?id=35436) by Rainer Noack: > Remove the local property and it's setter-method. Allow setting of the parent only in the constructors. Use `ClassLoader`'s parent property. Advantages: IMHO, the cleanest solution. Disadvantages: Many build brakes. Fazit: Not usable The latest comment by @bodewig states: > I don't think we would achieve much by setting `ClassLoader.parent` […]. In order to close this issue I've simply added a `getConfiguredParent` method in svn revision 796649 This was Solution 2 described in the original bug by Rainer Noack: > Provide a "special" getter like `getRealParent()` Advantages: works with existing environments. Disadvantages: such a shame Fazit: No better way !? So Stefan decided to go in a different, but equally valid, direction from Kohsuke. `AntClassLoader` has a mutable `parent` field, but its (no pun intended) parent `ClassLoader` class has an immutable `parent` field that can only be set by the constructor. The `getParent()` method is `final`, so `AntClassLoader` can't override it to return `AntClassLoader`'s mutable `parent` field. So @bodewig added a new `getConfiguredParent()` method which returns `AntClassLoader`'s mutable `parent` field. This makes sense, but the problem is that this change was incomplete. Other code still calls `getParent()` in several places. In particular, consider the code Kohsuke referred to in his analysis in JENKINS-22310: ```java if (parent != null && (!parentHasBeenSearched || parent != getParent())) { // Delegate to the parent: base = parent.getResources(name); // Note: could cause overlaps in case // ClassLoader.this.parent has matches and // parentHasBeenSearched is true } else { // ClassLoader.this.parent is already delegated to for example from // ClassLoader.getResources, no need: base = Collections.emptyEnumeration(); } ``` This calls `getParent()`, which will return the immutable `parent` field from the parent `ClassLoader` class rather than `AntClassLoader`'s mutable `parent` field. This is clearly a bug in `AntClassLoader`. This code should be using `getConfiguredParent()`. Sure enough, with this change I can no longer reproduce the problem in Jenkins: ```java @@ -961,7 +967,7 @@ public class AntClassLoader extends ClassLoader implements SubBuildListener, Clo throws IOException { final Enumeration<URL> mine = new ResourceEnumeration(name); Enumeration<URL> base; - if (parent != null && (!parentHasBeenSearched || parent != getParent())) { + if (parent != null && (!parentHasBeenSearched || parent != getConfiguredParent())) { // Delegate to the parent: base = parent.getResources(name); // Note: could cause overlaps in case ``` There's still another problematic use of `getParent()` in `getRootLoader()`. This doesn't appear to be a problem for Jenkins, but it is also technically incorrect. `getRootLoader()` recursively walks the `ClassLoader` hierarchy via `getParent()` until it reaches the root node. But if any of those `ClassLoader`s happen to be an `AntClassLoader` with a broken `getParent()` method, that won't work. The solution is to treat `AntClassLoader` specially: ```java @@ -841,12 +839,20 @@ public class AntClassLoader extends ClassLoader implements SubBuildListener, Clo */ private ClassLoader getRootLoader() { ClassLoader ret = getClass().getClassLoader(); - while (ret != null && ret.getParent() != null) { - ret = ret.getParent(); + while (ret != null && getParent(ret) != null) { + ret = getParent(ret); } return ret; } + private static ClassLoader getParent(ClassLoader cl) { + if (cl instanceof AntClassLoader) { + return ((AntClassLoader) cl).getConfiguredParent(); + } else { + return cl.getParent(); + } + } + ``` I tested this change with Jenkins, and Jenkins seems to work fine with it (although it was never broken in the first place). In my opinion, @bodewig's change in 2009 that introduced `getConfiguredParent()` should have updated both of these callers. It's not to late to do it now in 2021. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
