Velocity
  1. Velocity
  2. VELOCITY-196

ClasspathResourceLoader uses wrong ClassLoader

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.3.1
    • Fix Version/s: 1.5
    • Component/s: Engine
    • Labels:
      None
    • Environment:
      Operating System: other
      Platform: Other

      Description

      I see the following problems with ClasspathResourceLoader:

      1. It loads templates using the ClassLoader it was loaded in, which in many
      cases will be the top-level or a high-level ClassLoader within an appserver
      2. Individual applications within the server almost always are given their own
      contextual ClassLoader. In Tomcat, for example, each webapp has its own loader.
      3. Each webapp should be able to package templates in jars to be put in
      WEB-INF/lib and have Velocity load them correctly.
      4. Each webapp should not have to put velocity-xxx.jar in their WEB-INF/lib to
      be able to use Velocity.
      5. Therefore, Velocity should be able to load templates using the contextual
      ClassLoader appropriate for the caller

      ClasspathResourceLoader uses getClass().getClassLoader() to look for templates.
      If Velocity is loaded at the system or server level, and a webapp hopes to use
      this method to load its own jarred templates, it will fail as it did for me.

      The fix is terribly simple, and I think it makes sense to apply it directly to
      ClasspathResourceLoader, or at least provide a ThreadContextResourceLoader, as I
      have done:

      In ClasspathResourceLoader.getResourceStream(), change:

      ClassLoader classLoader = this.getClass().getClassLoader();

      to:

      ClassLoader classLoader = Thread.currentThread().getContextClassLoader();

      This will cause the resource loading to look first at the caller's level
      (webapp) and if not found, move up the ClassLoader chain. I believe this is the
      appropriate way to handle loading templates as resources. From the documentation
      for getContextClassLoader():

      ...

      • Returns the context ClassLoader for this Thread. The context
      • ClassLoader is provided by the creator of the thread for use
      • by code running in this thread when loading classes and resources.
      • If not set, the default is the ClassLoader context of the parent
      • Thread. The context ClassLoader of the primordial thread is
      • typically set to the class loader used to load the application.
        ...

      The resource should be loaded from the ClassLoader that the creator of the
      calling thread intended...not from the classloader that loaded Velocity. Also
      keep in mind this change would not break loading from the system classpath...it
      would just include contextual ClassLoaders in the lookup process.

      I will look into creating a patch if it would be useful, but it's a one-line fix.

      1. ASF.LICENSE.NOT.GRANTED--ClasspathFix.patch
        0.7 kB
        Charles Oliver Nutter
      2. ASF.LICENSE.NOT.GRANTED--patch.txt
        1 kB
        Will Glass-Husain

        Activity

        Hide
        Charles Oliver Nutter added a comment -

        Created an attachment (id=8054)
        Simple fix, untested in ClassPathResourceLoader, but works in alternative class that is otherwise identical

        Show
        Charles Oliver Nutter added a comment - Created an attachment (id=8054) Simple fix, untested in ClassPathResourceLoader, but works in alternative class that is otherwise identical
        Hide
        Daniel Rall added a comment -

        Charles, do you see any relation between (recently fixed) bug 8268 and this issue?

        Show
        Daniel Rall added a comment - Charles, do you see any relation between (recently fixed) bug 8268 and this issue?
        Hide
        Charles Oliver Nutter added a comment -

        There's probably some relation, but there's a deeper issue that lies under both
        bugs: What ClassLoader is used to load resources and classes.

        I can think of very few instances where the calling thread's preferred
        classloader shouldn't be used. In my case, for bug 22419, I created a
        ThreadContextResourceLoader that was identical to ClasspathResourceLoader except
        for the modification in the patch and the class name. However, in order for this
        class to be picked up, I had to put it at the system level because when looking
        for configured resource loaders, Velocity has the same problem: it only tries
        looking in the classloader it was loaded in (a la getClass().getClassLoader()).
        In just about any case where a classloader is needed to dynamically load a class
        or resource, Velocity should still request a classloader from the calling thread
        (as in the submitted patch).

        Also, modifying code to use the correct classloader should have no effect on
        current behavior. If the code that's dynamically loading resources is reachable,
        then it is at a higher classloader level then the code calling it. If the
        resource is not found at the lower level, the application server should allow
        classloading to fall through to higher levels.

        This patch just fixes ClasspathResourceLoader, which was my primary concern. It
        would be worth looking for other dynamic class/resource loading code and see if
        a similar change is necessary.

        Show
        Charles Oliver Nutter added a comment - There's probably some relation, but there's a deeper issue that lies under both bugs: What ClassLoader is used to load resources and classes. I can think of very few instances where the calling thread's preferred classloader shouldn't be used. In my case, for bug 22419, I created a ThreadContextResourceLoader that was identical to ClasspathResourceLoader except for the modification in the patch and the class name. However, in order for this class to be picked up, I had to put it at the system level because when looking for configured resource loaders, Velocity has the same problem: it only tries looking in the classloader it was loaded in (a la getClass().getClassLoader()). In just about any case where a classloader is needed to dynamically load a class or resource, Velocity should still request a classloader from the calling thread (as in the submitted patch). Also, modifying code to use the correct classloader should have no effect on current behavior. If the code that's dynamically loading resources is reachable, then it is at a higher classloader level then the code calling it. If the resource is not found at the lower level, the application server should allow classloading to fall through to higher levels. This patch just fixes ClasspathResourceLoader, which was my primary concern. It would be worth looking for other dynamic class/resource loading code and see if a similar change is necessary.
        Hide
        Charles Oliver Nutter added a comment -

        No movement on this? I'd sure like to get rid of my custom resource loader. It
        sounded like everyone agreed with this change on the mailing list.

        Show
        Charles Oliver Nutter added a comment - No movement on this? I'd sure like to get rid of my custom resource loader. It sounded like everyone agreed with this change on the mailing list.
        Hide
        Will Glass-Husain added a comment -

        Hi,

        I just wanted to note this is really a significant bug in
        ClassPathResourceLoader. Caching just doesn't work properly with the current
        implementation. This has been discussed recently on the velocity-user list,
        where another user came to the same conclusion.
        http://www.mail-archive.com/velocity-user%40jakarta.apache.org/msg12966.html

        Also, this thread on the Tomcat list is relevant.
        http://archives.real-time.com/pipermail/tomcat-users/2003-June/113209.html

        I suggest we apply this one-line fix and save future users the trouble of
        building their own resource loader (which is what the poster to velocity-user
        did).

        Show
        Will Glass-Husain added a comment - Hi, I just wanted to note this is really a significant bug in ClassPathResourceLoader. Caching just doesn't work properly with the current implementation. This has been discussed recently on the velocity-user list, where another user came to the same conclusion. http://www.mail-archive.com/velocity-user%40jakarta.apache.org/msg12966.html Also, this thread on the Tomcat list is relevant. http://archives.real-time.com/pipermail/tomcat-users/2003-June/113209.html I suggest we apply this one-line fix and save future users the trouble of building their own resource loader (which is what the poster to velocity-user did).
        Hide
        Teemu Kanstrén added a comment -

        I discussed this topic on the user list with Will, it is the link he mentioned.
        I would just like to add that the ThreadContext one line patch does not fix the
        caching problem. To fix the caching you must change this

        result= classLoader.getResourceAsStream( name );

        to this

        URL url = classLoader.getResource(name);
        result = url.openStream();

        see the link given by Will for more discussion.

        Show
        Teemu Kanstrén added a comment - I discussed this topic on the user list with Will, it is the link he mentioned. I would just like to add that the ThreadContext one line patch does not fix the caching problem. To fix the caching you must change this result= classLoader.getResourceAsStream( name ); to this URL url = classLoader.getResource(name); result = url.openStream(); see the link given by Will for more discussion.
        Hide
        Mike Heath added a comment -

        Is anything happening with this bug? I just spent a full day on this problem
        only to independently come to the exact same conclusion Charles came to and my
        change is exactly like the change Charles has submitted. (Great minds think
        alike. Good work, Charles.)

        The current implementation is very ignorant as to how the Java class loader
        works (especially when using J2EE containers.) I do not see this as a "minor"
        bug but as at least a "major" bug.

        Show
        Mike Heath added a comment - Is anything happening with this bug? I just spent a full day on this problem only to independently come to the exact same conclusion Charles came to and my change is exactly like the change Charles has submitted. (Great minds think alike. Good work, Charles.) The current implementation is very ignorant as to how the Java class loader works (especially when using J2EE containers.) I do not see this as a "minor" bug but as at least a "major" bug.
        Hide
        Will Glass-Husain added a comment -

        I've been looking in to this. First of all, there's really two separate
        issues in this bug. (1) the wrong classloader and (2) the caching. It turns
        out the reason caching doesn't work is that it's not implemented. More
        specifically, isSourceModified always returns false and getLastModified always
        returns 0.

        I'll post more later (on the dev list first), but I wanted to note that the
        link to the discussion is now invalid. Here's a better one.

        http://nagoya.apache.org/eyebrowse/ReadMsg?listName=velocity-
        user@jakarta.apache.org&msgNo=13531

        Show
        Will Glass-Husain added a comment - I've been looking in to this. First of all, there's really two separate issues in this bug. (1) the wrong classloader and (2) the caching. It turns out the reason caching doesn't work is that it's not implemented. More specifically, isSourceModified always returns false and getLastModified always returns 0. I'll post more later (on the dev list first), but I wanted to note that the link to the discussion is now invalid. Here's a better one. http://nagoya.apache.org/eyebrowse/ReadMsg?listName=velocity- user@jakarta.apache.org&msgNo=13531
        Hide
        Will Glass-Husain added a comment -

        Hi,

        I did a little research into this issue. Thanks to Mike Heath and Geir
        Magnusson for comments on the dev list on a draft of this posting. Hope this
        is helpful.

        Quick abstract. (a) Proposed solution seems reasonable; (b) Changing
        getResource to getResourceStream isn't required; (c) major caveat: the above
        change breaks Texen (when using ClasspathResourceLoader) due to ant weirdness,
        so need a fallback to old method if resource not found.

        (1) The problems:

        (a) Current design requires the templates to be loaded by the same ClassLoader
        as the Velocity jar. This prevents the user from putting the Velocity jar at
        the app server level and the templates at the WEB-INF/classes level. Although
        it technically may not apply, this is in violation of the servlet spec (at
        least in philosophy). [2]

        (b) Caching doesn't permit reloading of changed templates. I used to think
        this was related to (a), but actually it's just not implemented in the loader.

        (2) The proposed solution:

        Change:
        ClassLoader classLoader = this.getClass().getClassLoader();
        result= classLoader.getResourceAsStream( name );

        To:
        ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
        if (classLoader == null)

        { classLoader = this.getClass().getClassLoader(); result= classLoader.getResourceAsStream( name ); }

        else {
        result= classLoader.getResourceAsStream( name );
        if (result == null)

        { classLoader = this.getClass().getClassLoader(); result= classLoader.getResourceAsStream( name ); }

        }

        (3) Why "Thread.getContextClassLoader" instead of "Class.getClassLoader". Uses
        the classloader for the current thread, previously set with
        setContextClassLoader, rather than the class loader used to load the current
        class. An app server like Tomcat, for example, may load a class from the
        container level but will set the request thread context class loader to the
        web app level class loader.[3]

        Note that we're still ok with threads in which Thread.setContextClassLoader
        hasn't been called. In such cases, the default return value is the same as
        Class.getClassLoader() [4]. There's a miniscule chance this might be null, if
        a faulty app server previously calls Thread.setContextClassLoader(null).

        (4) Why not ClassLoader.getResource() instead of
        ClassLoader.getResourceAsStream() (as previously proposed). There was some
        speculation that the cache reload problem was due to getResource vs
        getResourceAsStream. [5] But actually, that can't be true since (as I said)
        caching is not implemented in the ClasspathResourceLoader. The email on the
        Tomcat list cited above is a red herring.

        (5) How to implement cache reloading. Right now, I'm thinking we shouldn't,
        because there is no true way to get the last modified time of a resource
        retrieved via a class. However, if we wanted to do a special case of when the
        resource is stored in a "classes" directory (as opposed to archived in a jar
        file), we could get the path with getResource().getFile() and check the mod
        time of the file. But I'm thinking this is too risky, as you really don't
        know if the ClassLoader is retrieving the files from a local path. Any
        thoughts on that?

        (6) Here's a problem. Any use of this Thread.currentThread
        ().getContextClassLoader() within an ant task (such as Texen) breaks. That
        means without the last 2 code lines, "ant test-texen-classpath" can't find the
        file. Apparently, with ant 1.5 the method call returns the primordial class
        loader (should be ok). With ant 1.6 (which I use), you see the class loader
        for the ant jar files. [6] [7] With the corrected version (if result ==
        null) "ant test" passes.

        NOTES

        [2]
        Servlet 2.3 spec
        "It is recommended also that the application class loader be implemented so
        that classes and resources packaged within the WAR are loaded in preference to
        classes and resources residing in container-wide library JARs." (p. 63)

        [3]
        Tomcat 4.0 plus. Tomcat 3.2 didn't do this since it worked with JDK 1.1,
        which was missing the setContextClassLoader method call. Tomcat 4+ requires
        Java 2, which has the method call.
        http://www.mail-archive.com/tomcat-user@jakarta.apache.org/msg04227.html

        [4]
        "Returns the context ClassLoader for this Thread. The context ClassLoader is
        provided by the creator of the thread for use by code running in this thread
        when loading classes and resources. If not set, the default is the ClassLoader
        context of the parent Thread. The context ClassLoader of the primordial thread
        is typically set to the class loader used to load the application."
        http://java.sun.com/j2se/1.4.2/docs/api/java/lang/Thread.html#getContextClassLo
        ader()

        [5]
        http://nagoya.apache.org/eyebrowse/ReadMsg?listId=103&msgNo=13515

        [6]
        http://nagoya.apache.org/eyebrowse/ReadMsg?
        listName=user@ant.apache.org&msgId=1113798

        [7]
        Not sure if this is related...

        http://issues.apache.org/bugzilla/show_bug.cgi?id=15018

        Show
        Will Glass-Husain added a comment - Hi, I did a little research into this issue. Thanks to Mike Heath and Geir Magnusson for comments on the dev list on a draft of this posting. Hope this is helpful. Quick abstract. (a) Proposed solution seems reasonable; (b) Changing getResource to getResourceStream isn't required; (c) major caveat: the above change breaks Texen (when using ClasspathResourceLoader) due to ant weirdness, so need a fallback to old method if resource not found. (1) The problems: (a) Current design requires the templates to be loaded by the same ClassLoader as the Velocity jar. This prevents the user from putting the Velocity jar at the app server level and the templates at the WEB-INF/classes level. Although it technically may not apply, this is in violation of the servlet spec (at least in philosophy). [2] (b) Caching doesn't permit reloading of changed templates. I used to think this was related to (a), but actually it's just not implemented in the loader. (2) The proposed solution: Change: ClassLoader classLoader = this.getClass().getClassLoader(); result= classLoader.getResourceAsStream( name ); To: ClassLoader classLoader = Thread.currentThread().getContextClassLoader(); if (classLoader == null) { classLoader = this.getClass().getClassLoader(); result= classLoader.getResourceAsStream( name ); } else { result= classLoader.getResourceAsStream( name ); if (result == null) { classLoader = this.getClass().getClassLoader(); result= classLoader.getResourceAsStream( name ); } } (3) Why "Thread.getContextClassLoader" instead of "Class.getClassLoader". Uses the classloader for the current thread, previously set with setContextClassLoader, rather than the class loader used to load the current class. An app server like Tomcat, for example, may load a class from the container level but will set the request thread context class loader to the web app level class loader. [3] Note that we're still ok with threads in which Thread.setContextClassLoader hasn't been called. In such cases, the default return value is the same as Class.getClassLoader() [4] . There's a miniscule chance this might be null, if a faulty app server previously calls Thread.setContextClassLoader(null). (4) Why not ClassLoader.getResource() instead of ClassLoader.getResourceAsStream() (as previously proposed). There was some speculation that the cache reload problem was due to getResource vs getResourceAsStream. [5] But actually, that can't be true since (as I said) caching is not implemented in the ClasspathResourceLoader. The email on the Tomcat list cited above is a red herring. (5) How to implement cache reloading. Right now, I'm thinking we shouldn't, because there is no true way to get the last modified time of a resource retrieved via a class. However, if we wanted to do a special case of when the resource is stored in a "classes" directory (as opposed to archived in a jar file), we could get the path with getResource().getFile() and check the mod time of the file. But I'm thinking this is too risky, as you really don't know if the ClassLoader is retrieving the files from a local path. Any thoughts on that? (6) Here's a problem. Any use of this Thread.currentThread ().getContextClassLoader() within an ant task (such as Texen) breaks. That means without the last 2 code lines, "ant test-texen-classpath" can't find the file. Apparently, with ant 1.5 the method call returns the primordial class loader (should be ok). With ant 1.6 (which I use), you see the class loader for the ant jar files. [6] [7] With the corrected version (if result == null) "ant test" passes. NOTES [2] Servlet 2.3 spec "It is recommended also that the application class loader be implemented so that classes and resources packaged within the WAR are loaded in preference to classes and resources residing in container-wide library JARs." (p. 63) [3] Tomcat 4.0 plus. Tomcat 3.2 didn't do this since it worked with JDK 1.1, which was missing the setContextClassLoader method call. Tomcat 4+ requires Java 2, which has the method call. http://www.mail-archive.com/tomcat-user@jakarta.apache.org/msg04227.html [4] "Returns the context ClassLoader for this Thread. The context ClassLoader is provided by the creator of the thread for use by code running in this thread when loading classes and resources. If not set, the default is the ClassLoader context of the parent Thread. The context ClassLoader of the primordial thread is typically set to the class loader used to load the application." http://java.sun.com/j2se/1.4.2/docs/api/java/lang/Thread.html#getContextClassLo ader() [5] http://nagoya.apache.org/eyebrowse/ReadMsg?listId=103&msgNo=13515 [6] http://nagoya.apache.org/eyebrowse/ReadMsg? listName=user@ant.apache.org&msgId=1113798 [7] Not sure if this is related... http://issues.apache.org/bugzilla/show_bug.cgi?id=15018
        Hide
        Will Glass-Husain added a comment -

        Created an attachment (id=13617)
        patch to correspond with comment #9 (ant test ok)

        Show
        Will Glass-Husain added a comment - Created an attachment (id=13617) patch to correspond with comment #9 (ant test ok)
        Hide
        Charles Oliver Nutter added a comment -

        Replies, by the numbers...

        (1) The summary in (a) sizes it up nicely. The caching issue is entirely
        separate, and was not a focus of my original bug entry. I think it should be
        handled elsewhere.

        (2) That seems like a reasonable fix. My original patch was simpler because I'm
        used to containers that set context threads correctly, but I understand and
        fully appreciate the need for a fallback case in misbehaving containers.

        (3) Correct about why Thread.getContextClassLoader. Also correct about the
        miniscule chance of a null context classloader, though I've certainly never seen
        it. There's also the additional case that security policies won't allow
        retrieving the context classloader...if you add a check for null, you should
        probably add a check for security constraints. Personally I don't think either
        is really necessary; if it ain't broke, don't fix it.

        (4) Correct; there is no difference between the two options, except for the
        obvious interface differences.

        (5) I think caching should be handled as a separate issue and not bog down the
        classloader concerns. You're right...they're unrelated, and the caching issue is
        much trickier.

        My only input here is that the cache should be maintained at the context
        classloader level; not at the Velocity classloader level. A good Web or J2EE
        container will already handle flushing out classloaders associated with an
        application when resources change, which covers 99% of your caching concerns.
        Make sure caching is done at the context classloader level instead of at a
        global level, and you gain the benefits of the container. Note: Most Jakarta
        projects with caches do not currently cache this way; they almost all cache
        using static fields in system-loaded classes (singleton caches), which
        effectively ties the cache to the system classloader and kills any sort of clean
        reloading. 'Bean utils' comes especially to mind, since it caches reflected data
        at the system level...a Very Bad Thing when the reflected classes are loaded by
        child classloaders. 'Nuff said.

        (6) I agree with the fallback changes to the original fix. I think, however,
        this qualifies as an Ant bug. Tasks should run in the context of a build, and
        should have all that build's classes and jars present as appropriate.
        getContextClassLoader should return a classloader appropriate for a task to run
        in the context of that build; it should never return Ant's classloader. At any
        rate, the fallback still makes good sense.

        I am giddy with anticipation. I'd also be more than willing to discuss or help
        out with the caching concerns.

        Show
        Charles Oliver Nutter added a comment - Replies, by the numbers... (1) The summary in (a) sizes it up nicely. The caching issue is entirely separate, and was not a focus of my original bug entry. I think it should be handled elsewhere. (2) That seems like a reasonable fix. My original patch was simpler because I'm used to containers that set context threads correctly, but I understand and fully appreciate the need for a fallback case in misbehaving containers. (3) Correct about why Thread.getContextClassLoader. Also correct about the miniscule chance of a null context classloader, though I've certainly never seen it. There's also the additional case that security policies won't allow retrieving the context classloader...if you add a check for null, you should probably add a check for security constraints. Personally I don't think either is really necessary; if it ain't broke, don't fix it. (4) Correct; there is no difference between the two options, except for the obvious interface differences. (5) I think caching should be handled as a separate issue and not bog down the classloader concerns. You're right...they're unrelated, and the caching issue is much trickier. My only input here is that the cache should be maintained at the context classloader level; not at the Velocity classloader level. A good Web or J2EE container will already handle flushing out classloaders associated with an application when resources change, which covers 99% of your caching concerns. Make sure caching is done at the context classloader level instead of at a global level, and you gain the benefits of the container. Note: Most Jakarta projects with caches do not currently cache this way; they almost all cache using static fields in system-loaded classes (singleton caches), which effectively ties the cache to the system classloader and kills any sort of clean reloading. 'Bean utils' comes especially to mind, since it caches reflected data at the system level...a Very Bad Thing when the reflected classes are loaded by child classloaders. 'Nuff said. (6) I agree with the fallback changes to the original fix. I think, however, this qualifies as an Ant bug. Tasks should run in the context of a build, and should have all that build's classes and jars present as appropriate. getContextClassLoader should return a classloader appropriate for a task to run in the context of that build; it should never return Ant's classloader. At any rate, the fallback still makes good sense. I am giddy with anticipation. I'd also be more than willing to discuss or help out with the caching concerns.
        Hide
        Will Glass-Husain added a comment -

        Patch applied. I'd still like to do something about the caching, but that's a
        new issue. Revision # 124212

        Show
        Will Glass-Husain added a comment - Patch applied. I'd still like to do something about the caching, but that's a new issue. Revision # 124212
        Hide
        Henning Schmiedehausen added a comment -

        Close all resolved issues for Engine 1.5 release.

        Show
        Henning Schmiedehausen added a comment - Close all resolved issues for Engine 1.5 release.
        Hide
        David Campbell added a comment -

        I find that the fix doesn't work, at least from an EJB deployed in the Glassfish application server. It can't load its templates.

        Show
        David Campbell added a comment - I find that the fix doesn't work, at least from an EJB deployed in the Glassfish application server. It can't load its templates.
        Hide
        David Campbell added a comment -

        Ignore my last comment, is wrong.

        Show
        David Campbell added a comment - Ignore my last comment, is wrong.

          People

          • Assignee:
            Unassigned
            Reporter:
            Charles Oliver Nutter
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development