You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@velocity.apache.org by Will Glass-Husain <wg...@forio.com> on 2004/11/28 07:51:34 UTC
ClasspathResourceLoader comments
Hi,
I did a little research into the ClasspathResourceLoader bugzilla entry [1].
Here is a summary of what I found. I'd appreciate any comments. If this
seems reasonable, I'll add it to the Bugzilla entry.
Quick abstract. (a) Proposed solution seems reasonable; (b) Changing
getResource to getResourceStream isn't required; (c) major caveat: this
change breaks Texen (when using ClasspathResourceLoader) due to ant
weirdness.
(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 );
(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 ClasspathResourceLoader within a
custom ant task (such as Texen) breaks. In the "ant test" suite, 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. Does
this mean that we should mark this bug DONT_FIX? It'd be nice to see a
consensus rather than letting this bug hang open.
[6] [7]
I'd appreciate any feedback - thanks!
WILL
NOTES
[1]
http://issues.apache.org/bugzilla/show_bug.cgi?id=22419
[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#getContextClassLoader()
[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
---------------------------------------------------------------------
To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
Re: ClasspathResourceLoader comments
Posted by Will Glass-Husain <wg...@forio.com>.
Good. I like the idea of a fallback myself. (Can't find the resource - do
it the old way).
Regarding Geir's comment of what might break. Would there be a situation in
which a false positive is an issue? In other words, is there a case where
you want the resource loader to fail? (Except it doesn't because the
resource exists at the primordial class loader level). Seems a strange case
to me.
Also, any comments on the caching issue? Specifically, we could add caching
for limited cases by translating a class to a filename (when applicable).
As I said in my email, I'm uncomfortable with this but wanted to raise the
possibility nonetheless. Regardless, we'll need to add to the
ClasspathResourceLoader docs that cache reloading doesn't work. (I found
this personally confusing when I first used it).
Best,
WILL
----- Original Message -----
From: "Geir Magnusson Jr." <ge...@optonline.net>
To: "Velocity Developers List" <ve...@jakarta.apache.org>
Cc: "Will Glass-Husain" <wg...@forio.com>
Sent: Monday, November 29, 2004 1:18 PM
Subject: Re: ClasspathResourceLoader comments
>
> On Nov 29, 2004, at 12:27 PM, Mike Heath wrote:
>
>>> (6) Here's a problem. Any use of this ClasspathResourceLoader within a
>>> custom ant task (such as Texen) breaks. In the "ant test" suite, 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. Does
>>> this mean that we should mark this bug DONT_FIX? It'd be nice to see a
>>> consensus rather than letting this bug hang open.
>>
>> It is important to note that the classloader problem exists for many
>> container based architectures. I discovered the problem using Apache
>> James (which is a Phoenix based application) so it is not just a
>> Tomcat, servlet or J2EE problem.
>>
>> Compatibility with Ant is certainly import but I feel strongly that
>> marking this bug as "DONT_FIX" would be ignoring the fact that the
>> ClasspathResourceLoader *is* broken as it stands now.
>>
>> To maintain compatibility with Ant, I would propose the following:
>>
>> 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 );
>> }
>> }
>>
>> This way the context class loader is used and if the resource is not
>> found, it falls back on the class' class loader.
>
> This was my thought - but I was just trying to figure out what we'd break
> if we did that...
>
>>
>> -Mike
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
>> For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
>>
>>
> --
> Geir Magnusson Jr +1-203-665-6437
> geir@gluecode.com
>
---------------------------------------------------------------------
To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
Re: ClasspathResourceLoader comments
Posted by "Geir Magnusson Jr." <ge...@optonline.net>.
On Nov 29, 2004, at 12:27 PM, Mike Heath wrote:
>> (6) Here's a problem. Any use of this ClasspathResourceLoader
>> within a
>> custom ant task (such as Texen) breaks. In the "ant test" suite, 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.
>> Does
>> this mean that we should mark this bug DONT_FIX? It'd be nice to see
>> a
>> consensus rather than letting this bug hang open.
>
> It is important to note that the classloader problem exists for many
> container based architectures. I discovered the problem using Apache
> James (which is a Phoenix based application) so it is not just a
> Tomcat, servlet or J2EE problem.
>
> Compatibility with Ant is certainly import but I feel strongly that
> marking this bug as "DONT_FIX" would be ignoring the fact that the
> ClasspathResourceLoader *is* broken as it stands now.
>
> To maintain compatibility with Ant, I would propose the following:
>
> 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 );
> }
> }
>
> This way the context class loader is used and if the resource is not
> found, it falls back on the class' class loader.
This was my thought - but I was just trying to figure out what we'd
break if we did that...
>
> -Mike
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
>
>
--
Geir Magnusson Jr +1-203-665-6437
geir@gluecode.com
---------------------------------------------------------------------
To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
Re: ClasspathResourceLoader comments
Posted by Mike Heath <el...@gmail.com>.
> (6) Here's a problem. Any use of this ClasspathResourceLoader within a
> custom ant task (such as Texen) breaks. In the "ant test" suite, 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. Does
> this mean that we should mark this bug DONT_FIX? It'd be nice to see a
> consensus rather than letting this bug hang open.
It is important to note that the classloader problem exists for many
container based architectures. I discovered the problem using Apache
James (which is a Phoenix based application) so it is not just a
Tomcat, servlet or J2EE problem.
Compatibility with Ant is certainly import but I feel strongly that
marking this bug as "DONT_FIX" would be ignoring the fact that the
ClasspathResourceLoader *is* broken as it stands now.
To maintain compatibility with Ant, I would propose the following:
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 );
}
}
This way the context class loader is used and if the resource is not
found, it falls back on the class' class loader.
-Mike
---------------------------------------------------------------------
To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: velocity-dev-help@jakarta.apache.org