Re: Attacking M5 - JIRA cleanup

[Aaron Mulder <am...@alumni.princeton.edu>]

On 9/28/05, Jeff Genender <jg...@savoirtech.com> wrote:
> > GERONIMO-518 - Deploying Struts app fails on Logging ClassCastException
> >
> > Aaron, looking at your latest comments this doesn't seems so easy to
> > solve.  I wonder if we shouldn't add this to the "Significant Missing
> > Features" section of our release notes.
>
> This is not a bug...I will probably close this out - unless others
> object.  This has to do with the commons logging jar in the web app.

I think I object.  If we feel that commons logging is part of the
server implementation and cannot be redefined in the web app, then we
should add it to the list of packages we refuse to load from the web
app classloader (so if someone puts it in there, we'll just silently
ignore it).  It seems pretty obnoxious to take Struts WARs that work
perfectly well with other app servers and claim that it's the user's
fault that they blow up in Geronimo, you know?

Aaron

Re: Attacking M5 - JIRA cleanup

[Kevan Miller <ke...@gmail.com>]

I've created an app which reproduces the problem described by
http://issues.apache.org/jira/browse/GERONIMO-518. The app is one of the
Struts example programs (mailreader) to which I've added a geronimo-web.xml.
Oh, and I also made a minor mod to its web.xml to get it to deploy under
Geronimo. I'll attach the war to the Jira. Note that the app can be
deployed/executed successfully with context-priority-classloader set to
false. However, if context-priority-classloader is set to true, the deploy
will fail with a ClassCastException.

I have a potential fix for the problem, which I'll also attach to the Jira.
IMO, the fix is as good as the current context-priority-classloader
implementation.

In the way of background, the Geronimo classloaders for both Jetty and
Tomcat have a number of packages that they exclude from being loaded from a
web app's context (instead, they'll always be loaded from using the parent
classloader). This behavior is as specified by the Servlet specification
(which states that java, javax, and "container implementation" classes
should not be loaded from a web app's context. The Jetty/Tomcat classloaders
currently exclude java.*, javax.*, org.apache.geronimo.*, etc. Users
currently have no way of predicting what packages will be excluded by the
context-priority-classloader. My fix simply adds
org.apache.commons.logging.* to this exclusion list (I think there are
probably other "implementation classes" which ought to be excluded, but I'm
not addressing these. Also, as Anita suggested, it would be useful to users
to inform them when a class has been "excluded". There are some potential
issues with this -- load times and potentially large number of messages -- I
haven't attempted to address.

I've tested this change under Jetty and the test app deploys without error,
and appears to work fine. Deployment under Tomcat fails with a
NullPointerException. However, I don't believe this has anything to do with
my fix. I'll pursue this problem separately.

I think the behavior of context-priority-classloader needs to be reviewed,
the package exclusion lists for Tomcat and Jetty needs to be reviewed, and
all of this needs to be documented for users. I think the current
implementation/exclusion list gives users a reasonable chance of introducing
deployment/runtime errors in their web app without a way of predicting this
behavior.

Finally, It's possible that there's a fundamental problem which is causing
the ClassCastException (I'm a relative novice when it comes to web
containers/classloaders/commons logging). So, it's probably worth reviewing
what's happening when we get the ClassCastException.

The code where the ClassCastException is occurring is in
o.a.commons.logging.LogFactory.newFactory(factoryClassName, classloader).
The stack trace line numbers don't exactly match with my source code, but
I'm reasonably certain of the following:

LogFactory newFactory(String factoryClass, Classloader classLoader) {
...
try {
try {
return (LogFactory) classLoader.loadClass(factoryClass).newInstance(); //
(1) the initial ClassCastException
}
catch (ClassCastException cce) {
if (classLoader == LogFactory.class.getClassLoader()) // (2) and yet this
check succeeds
throw cce; // (3) so the exception is thrown again
}
return (LogFactory) Class.forName(factoryClass).newInstance(); // and we
never get here...
}
catch (Exception e) {
throw new LogConfigurationException(); // (4) and ultimately fail here
}
}

I'm assuming that the initial ClassCastException (1) occurs because
(LogFactory) was not loaded by the parent classloader, not the context
classloader. Yet, I don't understand why (2), the subsequent classLoader ==
check, would then succeed. Thoughts anyone? I haven't debugged to the point
of validating the factoryClass etc. But I'm assuming this is not a struts
implementation/application issue.

--kevan

On 9/28/05, anita kulshreshtha <a_...@yahoo.com> wrote:
>
>
>
> --- Aaron Mulder <am...@alumni.princeton.edu>
> wrote:
>
> > On 9/28/05, Jeff Genender <jg...@savoirtech.com>
> > wrote:
> > > > GERONIMO-518 - Deploying Struts app fails on
> > Logging ClassCastException
> > > >
> > > > Aaron, looking at your latest comments this
> > doesn't seems so easy to
> > > > solve. I wonder if we shouldn't add this to the
> > "Significant Missing
> > > > Features" section of our release notes.
> > >
> > > This is not a bug...I will probably close this out
> > - unless others
> > > object. This has to do with the commons logging
> > jar in the web app.
> >
> > I think I object. If we feel that commons logging
> > is part of the
> > server implementation and cannot be redefined in the
> > web app, then we
> > should add it to the list of packages we refuse to
> > load from the web
> > app classloader (so if someone puts it in there,
> > we'll just silently
> > ignore it).
>
> A warning will be even better. Eventually G will have
> fixed version of all the jars. If a user needs to use
> a later version of a jar, for example to include a
> latest feature/bug fix important to their app), will
> they be asked to upgrade G or be allowed to override
> the jar.
>
> It seems pretty obnoxious to take
> > Struts WARs that work
> > perfectly well with other app servers and claim that
> > it's the user's
> > fault that they blow up in Geronimo, you know?
>
> I agree.
>
> Thanks
> Anita
> >
> > Aaron
> >
>
>
>
>
> __________________________________
> Yahoo! Mail - PC Magazine Editors' Choice 2005
> http://mail.yahoo.com
>

Re: Attacking M5 - JIRA cleanup

["Geir Magnusson Jr." <ge...@apache.org>]

On Sep 28, 2005, at 11:29 AM, Aaron Mulder wrote:

> On 9/28/05, Jeff Genender <jg...@savoirtech.com> wrote:
>
>>> GERONIMO-518 - Deploying Struts app fails on Logging  
>>> ClassCastException
>>>
>>> Aaron, looking at your latest comments this doesn't seems so easy to
>>> solve.  I wonder if we shouldn't add this to the "Significant  
>>> Missing
>>> Features" section of our release notes.
>>>
>>
>> This is not a bug...I will probably close this out - unless others
>> object.  This has to do with the commons logging jar in the web app.
>>
>
> I think I object.  If we feel that commons logging is part of the
> server implementation and cannot be redefined in the web app, then we
> should add it to the list of packages we refuse to load from the web
> app classloader (so if someone puts it in there, we'll just silently
> ignore it).  It seems pretty obnoxious to take Struts WARs that work
> perfectly well with other app servers and claim that it's the user's
> fault that they blow up in Geronimo, you know?

And people wonder why I hate commons logging...

geir

>
> Aaron
>
>

-- 
Geir Magnusson Jr                                  +1-203-665-6437
geirm@apache.org

Re: Attacking M5 - JIRA cleanup

[anita kulshreshtha <a_...@yahoo.com>]

--- Aaron Mulder <am...@alumni.princeton.edu>
wrote:

> On 9/28/05, Jeff Genender <jg...@savoirtech.com>
> wrote:
> > > GERONIMO-518 - Deploying Struts app fails on
> Logging ClassCastException
> > >
> > > Aaron, looking at your latest comments this
> doesn't seems so easy to
> > > solve.  I wonder if we shouldn't add this to the
> "Significant Missing
> > > Features" section of our release notes.
> >
> > This is not a bug...I will probably close this out
> - unless others
> > object.  This has to do with the commons logging
> jar in the web app.
> 
> I think I object.  If we feel that commons logging
> is part of the
> server implementation and cannot be redefined in the
> web app, then we
> should add it to the list of packages we refuse to
> load from the web
> app classloader (so if someone puts it in there,
> we'll just silently
> ignore it).

A warning will be even better. Eventually G will have 
fixed version of all the jars. If a user needs to use
a later version of a jar, for example to include a
latest feature/bug fix important to their app), will
they be asked to upgrade G or be allowed to override
the jar. 

 It seems pretty obnoxious to take
> Struts WARs that work
> perfectly well with other app servers and claim that
> it's the user's
> fault that they blow up in Geronimo, you know?

I agree.

Thanks
Anita
> 
> Aaron
> 



		
__________________________________ 
Yahoo! Mail - PC Magazine Editors' Choice 2005 
http://mail.yahoo.com