ant-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [ant] basil commented on pull request #149: Ant doesn't call the superclass constructor and pass in the specified parent classloader
Date Wed, 21 Jul 2021 16:20:47 GMT

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-unsubscribe@ant.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


Mime
View raw message