You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@struts.apache.org by James Mitchell <jm...@telocity.com> on 2002/11/07 17:13:35 UTC
[Vote] Modify classloading in RequestUtils
Not sure if you caught the thread on the users list, or the bug recently
posted by Kjeld Froberg:
http://nagoya.apache.org/bugzilla/show_bug.cgi?id=14332
Here's the proposed code change:
RequestUtils.java
-----------------------------------------------
From:
public static Class applicationClass(String className)
throws ClassNotFoundException {
// Look up the class loader to be used
ClassLoader classLoader =
Thread.currentThread().getContextClassLoader();
if (classLoader == null) {
classLoader = RequestUtils.class.getClassLoader();
}
// Attempt to load the specified class
return (classLoader.loadClass(className));
}
-----------------------------------------------
To:
public static Class applicationClass(String className)
throws ClassNotFoundException {
ClassLoader ctxLoader = null;
try {
ctxLoader = Thread.currentThread().getContextClassLoader();
return Class.forName(className, true, ctxLoader);
} catch(ClassNotFoundException ex) {
if(ctxLoader == null) { throw ex; }
} catch(SecurityException ex) {
}
return Class.forName(className);
}
By changing these few lines, I am able to run the full test suite without
error. (And that includes the new test.tomcat.33)
I'm not sure if this should be a [Vote] or a [commit and wait till they
complain], so I opted for the former.
Even though I'm not truly sure of the consequences of this change, I'm +1.
James Mitchell
Software Engineer/Struts Evangelist
http://www.open-tools.org
"Only two things are infinite, the universe and human stupidity, and I'm not
sure about the former."
- Albert Einstein (1879-1955)
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [Vote] Modify classloading in RequestUtils
Posted by Robert Leland <rl...@apache.org>.
See comments enclosed
James Mitchell wrote:
>Not sure if you caught the thread on the users list, or the bug recently
>posted by Kjeld Froberg:
>
> http://nagoya.apache.org/bugzilla/show_bug.cgi?id=14332
>
>
>Here's the proposed code change:
>
>RequestUtils.java
>-----------------------------------------------
>From:
> public static Class applicationClass(String className)
> throws ClassNotFoundException {
>
> // Look up the class loader to be used
> ClassLoader classLoader =
> Thread.currentThread().getContextClassLoader();
> if (classLoader == null) {
> classLoader = RequestUtils.class.getClassLoader();
> }
>
> // Attempt to load the specified class
> return (classLoader.loadClass(className));
> }
>-----------------------------------------------
>To:
> public static Class applicationClass(String className)
> throws ClassNotFoundException {
> ClassLoader ctxLoader = null;
> try {
> ctxLoader = Thread.currentThread().getContextClassLoader();
> return Class.forName(className, true, ctxLoader);
>
> } catch(ClassNotFoundException ex) {
> if(ctxLoader == null) { throw ex; }
> } catch(SecurityException ex) {
>
> }
> return Class.forName(className);
> }
>
>By changing these few lines, I am able to run the full test suite without
>error. (And that includes the new test.tomcat.33)
>
>I'm not sure if this should be a [Vote] or a [commit and wait till they
>complain], so I opted for the former.
>
I would be +0,--ONLY-- after some more digging.
Lets walk through this, here is my guess.
The patch would use the containers class loader first then the JVM's class
loader, the original code uses
the containers class loader then if that fails looks for the class in
the struts Jar.
About April 2001, I recall the 'Class.forName' method being frowned upon,
I would suggest
1) search the archives, if you can't find the reference about
Class.forName()
then I'll see if I can dig it up.
2) Take a look at the log of applicationClass() and determine why/when
RequestUtils.class.getClassLoader()
was added it could have been to work around a bug in another container.
-Rob
>
>Even though I'm not truly sure of the consequences of this change, I'm +1.
>
>
>James Mitchell
>
>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [Vote] Modify classloading in RequestUtils
Posted by James Mitchell <jm...@telocity.com>.
Yes, it is the same.
Thank you.
James Mitchell
Software Engineer/Struts Evangelist
http://www.open-tools.org
"Only two things are infinite, the universe and human stupidity, and I'm not
sure about the former."
- Albert Einstein (1879-1955)
> -----Original Message-----
> From: Antoni Reus [mailto:antoni.reus@wanadoo.es]
> Sent: Thursday, November 07, 2002 4:21 PM
> To: Struts Developers List
> Subject: Re: [Vote] Modify classloading in RequestUtils
>
>
> It seems that this is related with
>
> http://nagoya.apache.org/bugzilla/show_bug.cgi?id=13686
>
> a similar issue with iplanet
>
>
> A Dijous 07 Novembre 2002 17:13, James Mitchell va escriure:
> > Not sure if you caught the thread on the users list, or the bug recently
> > posted by Kjeld Froberg:
> >
> > http://nagoya.apache.org/bugzilla/show_bug.cgi?id=14332
> >
> >
> > Here's the proposed code change:
> >
> > RequestUtils.java
> > -----------------------------------------------
> > From:
> > public static Class applicationClass(String className)
> > throws ClassNotFoundException {
> >
> > // Look up the class loader to be used
> > ClassLoader classLoader =
> > Thread.currentThread().getContextClassLoader();
> > if (classLoader == null) {
> > classLoader = RequestUtils.class.getClassLoader();
> > }
> >
> > // Attempt to load the specified class
> > return (classLoader.loadClass(className));
> > }
> > -----------------------------------------------
> > To:
> > public static Class applicationClass(String className)
> > throws ClassNotFoundException {
> > ClassLoader ctxLoader = null;
> > try {
> > ctxLoader =
> Thread.currentThread().getContextClassLoader();
> > return Class.forName(className, true, ctxLoader);
> >
> > } catch(ClassNotFoundException ex) {
> > if(ctxLoader == null) { throw ex; }
> > } catch(SecurityException ex) {
> >
> > }
> > return Class.forName(className);
> > }
> >
> > By changing these few lines, I am able to run the full test
> suite without
> > error. (And that includes the new test.tomcat.33)
> >
> > I'm not sure if this should be a [Vote] or a [commit and wait till they
> > complain], so I opted for the former.
> >
> > Even though I'm not truly sure of the consequences of this
> change, I'm +1.
> >
> >
> > James Mitchell
> > Software Engineer/Struts Evangelist
> > http://www.open-tools.org
> >
> > "Only two things are infinite, the universe and human stupidity, and I'm
> > not sure about the former."
> > - Albert Einstein (1879-1955)
>
>
> --
> To unsubscribe, e-mail:
> <ma...@jakarta.apache.org>
> For additional commands, e-mail:
> <ma...@jakarta.apache.org>
>
>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [Vote] Modify classloading in RequestUtils
Posted by Antoni Reus <an...@wanadoo.es>.
It seems that this is related with
http://nagoya.apache.org/bugzilla/show_bug.cgi?id=13686
a similar issue with iplanet
A Dijous 07 Novembre 2002 17:13, James Mitchell va escriure:
> Not sure if you caught the thread on the users list, or the bug recently
> posted by Kjeld Froberg:
>
> http://nagoya.apache.org/bugzilla/show_bug.cgi?id=14332
>
>
> Here's the proposed code change:
>
> RequestUtils.java
> -----------------------------------------------
> From:
> public static Class applicationClass(String className)
> throws ClassNotFoundException {
>
> // Look up the class loader to be used
> ClassLoader classLoader =
> Thread.currentThread().getContextClassLoader();
> if (classLoader == null) {
> classLoader = RequestUtils.class.getClassLoader();
> }
>
> // Attempt to load the specified class
> return (classLoader.loadClass(className));
> }
> -----------------------------------------------
> To:
> public static Class applicationClass(String className)
> throws ClassNotFoundException {
> ClassLoader ctxLoader = null;
> try {
> ctxLoader = Thread.currentThread().getContextClassLoader();
> return Class.forName(className, true, ctxLoader);
>
> } catch(ClassNotFoundException ex) {
> if(ctxLoader == null) { throw ex; }
> } catch(SecurityException ex) {
>
> }
> return Class.forName(className);
> }
>
> By changing these few lines, I am able to run the full test suite without
> error. (And that includes the new test.tomcat.33)
>
> I'm not sure if this should be a [Vote] or a [commit and wait till they
> complain], so I opted for the former.
>
> Even though I'm not truly sure of the consequences of this change, I'm +1.
>
>
> James Mitchell
> Software Engineer/Struts Evangelist
> http://www.open-tools.org
>
> "Only two things are infinite, the universe and human stupidity, and I'm
> not sure about the former."
> - Albert Einstein (1879-1955)
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: RE: [Vote] Modify classloading in RequestUtils
Posted by Ted Husted <hu...@apache.org>.
I think this is starting to sound like a job for 1.2.x =:0)
-Ted.
11/7/2002 1:43:17 PM, "Craig R. McClanahan" <cr...@apache.org>
wrote:
>Personally, I would feel a lot better if we could try the
>modified version on lots of different servers (in addition to
>Tomcat), and with struts.jar either inside or outside the webapp
>(the tests above only put it inside).
<snip/>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [Vote] Modify classloading in RequestUtils
Posted by "Craig R. McClanahan" <cr...@apache.org>.
On Thu, 7 Nov 2002, James Mitchell wrote:
> Date: Thu, 7 Nov 2002 12:51:22 -0500
> From: James Mitchell <jm...@telocity.com>
> Reply-To: Struts Developers List <st...@jakarta.apache.org>
> To: Struts Developers List <st...@jakarta.apache.org>
> Subject: RE: [Vote] Modify classloading in RequestUtils
>
> Absolutely, I never intended to commit unless I had buy-in from everyone.
>
> I don't claim to understand the full impact of this, so your comments as
> well as Roberts are always welcome.
>
>
> By reverting the code (my local copy anyway) back to its original:
> test.junit <== SUCCESSFUL Total time: 16 seconds
> test.tomcat.32 <== NOT SUCCESSFUL (Details below)
> test.tomcat.33 <== SUCCESSFUL Total time: 54 seconds
> test.tomcat.40 <== SUCCESSFUL Total time: 53 seconds
> test.tomcat.41 <== SUCCESSFUL Total time: 59 seconds
Personally, I would feel a lot better if we could try the modified version
on lots of different servers (in addition to Tomcat), and with struts.jar
either inside or outside the webapp (the tests above only put it inside).
Can we figure out a way to:
* Build a patched distribution of Struts 1.1 nightlies with
this being the only change
* Design a simple test strategy that can be easily replicated
across the different servers and environments
* Volunteer to test and report back on one or more environments?
and verify that we're not going to destroy the ability to run Struts
someplace we haven't thought about yet?
Yes, we need to deal with Tomcat 3.2, and this patch does so -- but fixing
support on 3.2 is not worth breaking support on something else.
Craig
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [Vote] Modify classloading in RequestUtils
Posted by James Mitchell <jm...@telocity.com>.
Absolutely, I never intended to commit unless I had buy-in from everyone.
I don't claim to understand the full impact of this, so your comments as
well as Roberts are always welcome.
By reverting the code (my local copy anyway) back to its original:
test.junit <== SUCCESSFUL Total time: 16 seconds
test.tomcat.32 <== NOT SUCCESSFUL (Details below)
test.tomcat.33 <== SUCCESSFUL Total time: 54 seconds
test.tomcat.40 <== SUCCESSFUL Total time: 53 seconds
test.tomcat.41 <== SUCCESSFUL Total time: 59 seconds
...
...
[junit] Testcase: testInitDestroyInternal took 3.094 sec
[junit] Caused an ERROR
[junit] null
[junit] java.lang.NullPointerException
[junit] at
org.apache.struts.util.MessageResources.getMessageResources(MessageResources
.java:580)
[junit] at
org.apache.struts.action.ActionServlet.initInternal(ActionServlet.java:1145)
[junit] at
org.apache.struts.action.TestActionServlet.testInitDestroyInternal(TestActio
nServlet.java:112)
[junit] at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
...
...
The detail of the tomcat.32 failure lead back to the static call to:
RequestUtils.applicationClass(String className)
So, what should I do with bug 14332 right now?
James Mitchell
Software Engineer/Struts Evangelist
http://www.open-tools.org
"Only two things are infinite, the universe and human stupidity, and I'm not
sure about the former."
- Albert Einstein (1879-1955)
> -----Original Message-----
> From: Craig R. McClanahan [mailto:craigmcc@apache.org]
> Sent: Thursday, November 07, 2002 12:31 PM
> To: Struts Developers List
> Subject: Re: [Vote] Modify classloading in RequestUtils
>
>
> As a general principle, J2EE 1.4 (and later) requires app servers to make
> the webapp's class loader available via the getContextClassLoader() call.
> This feature is already supported by nearly every other server in the
> world. Therefore, I'd really recommend that the TriFork folks upgrade
> their server to do the same.
>
> Please don't make this change until we've had more time to evaluate it's
> potential impacts. I'm particularly concerned about the semantics of
> Class.forName() versus the current approach that uses the class loader
> that loaded RequestUtils itself. That needs to be looked at more
> carefully, particularly with respect to multi-class-loader hierarchies
> like the one Tomcat 4 supports.
>
> Craig
>
>
> On Thu, 7 Nov 2002, James Mitchell wrote:
>
> > Date: Thu, 7 Nov 2002 11:13:35 -0500
> > From: James Mitchell <jm...@telocity.com>
> > Reply-To: Struts Developers List <st...@jakarta.apache.org>
> > To: Struts Developers List <st...@jakarta.apache.org>
> > Subject: [Vote] Modify classloading in RequestUtils
> >
> >
> > Not sure if you caught the thread on the users list, or the bug recently
> > posted by Kjeld Froberg:
> >
> > http://nagoya.apache.org/bugzilla/show_bug.cgi?id=14332
> >
> >
> > Here's the proposed code change:
> >
> > RequestUtils.java
> > -----------------------------------------------
> > From:
> > public static Class applicationClass(String className)
> > throws ClassNotFoundException {
> >
> > // Look up the class loader to be used
> > ClassLoader classLoader =
> > Thread.currentThread().getContextClassLoader();
> > if (classLoader == null) {
> > classLoader = RequestUtils.class.getClassLoader();
> > }
> >
> > // Attempt to load the specified class
> > return (classLoader.loadClass(className));
> > }
> > -----------------------------------------------
> > To:
> > public static Class applicationClass(String className)
> > throws ClassNotFoundException {
> > ClassLoader ctxLoader = null;
> > try {
> > ctxLoader =
> Thread.currentThread().getContextClassLoader();
> > return Class.forName(className, true, ctxLoader);
> >
> > } catch(ClassNotFoundException ex) {
> > if(ctxLoader == null) { throw ex; }
> > } catch(SecurityException ex) {
> >
> > }
> > return Class.forName(className);
> > }
> >
> > By changing these few lines, I am able to run the full test
> suite without
> > error. (And that includes the new test.tomcat.33)
> >
> > I'm not sure if this should be a [Vote] or a [commit and wait till they
> > complain], so I opted for the former.
> >
> > Even though I'm not truly sure of the consequences of this
> change, I'm +1.
> >
> >
> > James Mitchell
> > Software Engineer/Struts Evangelist
> > http://www.open-tools.org
> >
> > "Only two things are infinite, the universe and human
> stupidity, and I'm not
> > sure about the former."
> > - Albert Einstein (1879-1955)
> >
> >
> > --
> > To unsubscribe, e-mail:
<ma...@jakarta.apache.org>
> For additional commands, e-mail:
<ma...@jakarta.apache.org>
>
>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [Vote] Modify classloading in RequestUtils
Posted by "Craig R. McClanahan" <cr...@apache.org>.
As a general principle, J2EE 1.4 (and later) requires app servers to make
the webapp's class loader available via the getContextClassLoader() call.
This feature is already supported by nearly every other server in the
world. Therefore, I'd really recommend that the TriFork folks upgrade
their server to do the same.
Please don't make this change until we've had more time to evaluate it's
potential impacts. I'm particularly concerned about the semantics of
Class.forName() versus the current approach that uses the class loader
that loaded RequestUtils itself. That needs to be looked at more
carefully, particularly with respect to multi-class-loader hierarchies
like the one Tomcat 4 supports.
Craig
On Thu, 7 Nov 2002, James Mitchell wrote:
> Date: Thu, 7 Nov 2002 11:13:35 -0500
> From: James Mitchell <jm...@telocity.com>
> Reply-To: Struts Developers List <st...@jakarta.apache.org>
> To: Struts Developers List <st...@jakarta.apache.org>
> Subject: [Vote] Modify classloading in RequestUtils
>
>
> Not sure if you caught the thread on the users list, or the bug recently
> posted by Kjeld Froberg:
>
> http://nagoya.apache.org/bugzilla/show_bug.cgi?id=14332
>
>
> Here's the proposed code change:
>
> RequestUtils.java
> -----------------------------------------------
> From:
> public static Class applicationClass(String className)
> throws ClassNotFoundException {
>
> // Look up the class loader to be used
> ClassLoader classLoader =
> Thread.currentThread().getContextClassLoader();
> if (classLoader == null) {
> classLoader = RequestUtils.class.getClassLoader();
> }
>
> // Attempt to load the specified class
> return (classLoader.loadClass(className));
> }
> -----------------------------------------------
> To:
> public static Class applicationClass(String className)
> throws ClassNotFoundException {
> ClassLoader ctxLoader = null;
> try {
> ctxLoader = Thread.currentThread().getContextClassLoader();
> return Class.forName(className, true, ctxLoader);
>
> } catch(ClassNotFoundException ex) {
> if(ctxLoader == null) { throw ex; }
> } catch(SecurityException ex) {
>
> }
> return Class.forName(className);
> }
>
> By changing these few lines, I am able to run the full test suite without
> error. (And that includes the new test.tomcat.33)
>
> I'm not sure if this should be a [Vote] or a [commit and wait till they
> complain], so I opted for the former.
>
> Even though I'm not truly sure of the consequences of this change, I'm +1.
>
>
> James Mitchell
> Software Engineer/Struts Evangelist
> http://www.open-tools.org
>
> "Only two things are infinite, the universe and human stupidity, and I'm not
> sure about the former."
> - Albert Einstein (1879-1955)
>
>
> --
> To unsubscribe, e-mail: <ma...@jakarta.apache.org>
> For additional commands, e-mail: <ma...@jakarta.apache.org>
>
>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>