You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by robert burrell donkin <ro...@blueyonder.co.uk> on 2003/11/02 23:02:26 UTC
Re: commons-logging & classloading (continued)
IMHO the right way to solve the issue of loading Log implementations is to
use the existing system property to load a different LogFactory
implementation. this implementation can then override LogFactoryImpl and
load using whatever classloader is most appropriate.
the issue then becomes how to load the initial LogFactory implementation.
it seems to me that at the moment given a suitable stuffed up classloading
configuration, the LogFactory implementation will be loaded by the context
classloader and not be assignable. the current LogFactory implementation
looks like it will throw a configuration exception even if a compatible
implementation of the class is available in the class classloader for
LogFactory.
i can see two improvements we might make for users faced with this
situation:
1. we could introduce a system property flag which (when set) would force
the LogFactory class classloader to be used.
2, a more sophisticated implementation for the loading of the LogFactory
implementation that would try to load the class from the LogFactory class
classloader if the assignment fails. since (at the moment) we throw an
exception in this circumstance (from would be very difficult to recover
from), i don't think that add this extra step would do any harm.
comments anyone?
BTW one feature that i've been wanting for some time is the ability to
turn off all configuration exceptions and go with a reliable default
(probably a LogFactory nested class). there are times when library code
failing to initialize logging is not a good enough reason to stop the
executing of the application. maybe a system property could be added for
this.
again, comments please.
- robert
On Thursday, October 30, 2003, at 10:36 AM, Ojares Rami EINT wrote:
> Well put Norbert.
> I think that since classloading and threads are such complex
> issues there should be a way to not use the pattern
> of loading the implementation from thread's context classloader.
> (Hint to Craig :)
>
> - rami
>
>> -----Original Message-----
>> From: Norbert Klose [mailto:nklose@mail.com]
>> Sent: Thursday, October 30, 2003 12:21 PM
>> To: commons-dev@jakarta.apache.org
>> Subject: commons-logging & classloading (continued)
>>
>>
>> Hello,
>>
>> i currently use Tomcat 4.1.27 bundled with commons-logging
>> 1.0.3. My own webapp i'm working on also uses commons-logging,
>> so i include a copy of the jar file into the WEB-INF/lib
>> directory to be protable to other servlet containers that does
>> not include the commons-logging package. I found some
>> discussions in the mail archive that is about commons-logging
>> and its class loading strategy. But as i could not found an
>> anwser to my problem, i post my problem here again, hoping to
>> get a hint for a solution (or maybe to settle on a new consens).
>>
>> The problem is, that when tomcat wants to anser a HTTPS
>> request it instantiates a Http11ConnectionHandler which
>> processes the connection.
>> The Http11ConnectionHandler instance itself instantiates a
>> JSSE14Support class which itself instantiates a
>> org.apache.commons.logging.Log implementation class. Because
>> the thread that runs the Http11ConnectionHandler has the
>> WebappClassloader of my web application as its
>> context class loader (which ist used by commons-logging to
>> load the Log implementation (logClass)), BUT the
>> org.apache.commons.logging.Log interface itself was loaded
>> from the Common StandardClassLoader, the predicate in
>> LogFactoryImpl.getLogConstructor()
>>
>> (!Log.class.isAssignableFrom(logClass))
>>
>> is false, so that LogFactoryImpl.getLogConstructor() throws a
>> LogConfigurationException. Because both classes are loaded
>> from different classloaders.
>>
>> IMHO what commons-logging MUST ensure to work correctly is,
>> that the logClass is loaded from the same classloader than the
>> Log.class is and this is not guaranteed by the current
>> implementation! For example
>>
>> protected static ClassLoader getContextClassLoader()
>> throws LogConfigurationException
>> {
>> return Log.class.getClassLoader();
>> }
>>
>> would do. BUT to keep the current implementation what about
>> changing LogFactoryImpl.getLogConstructor(), so that the
>> correct predicate is evaluated?
>>
>> protected Constructor getLogConstructor()
>> throws LogConfigurationException {
>>
>> ...
>>
>> logClass = loadClass(logClassName);
>> ___logClass___ =
>> loadClass("org.apache.commons.logging.Log") //
>> something like this...
>> if (logClass == null) {
>> throw new LogConfigurationException
>> ("No suitable Log implementation for " +
>> logClassName);
>> }
>> if (!___logClass___.class.isAssignableFrom(logClass)) {
>> ...
>> }
>>
>> ...
>>
>> }
>>
>> The problem with the current implementation is, that
>> commons-logging can not rely on the fact that the threads
>> current context class loader is the classloader that the class
>> (like the JSSE14Support above) wants to get its logging
>> implementation from!!!
>>
>> Is there a chance to get a consens on that, or at least to
>> think about changing the current implemetation making it more
>> reliable ?
>>
>> Kindly regards,
>> Norbert.
>> --
>> __________________________________________________________
>> Sign-up for your own personalized E-mail at Mail.com
>> http://www.mail.com/?sr=signup
>>
>> CareerBuilder.com has over 400,000 jobs. Be smarter about your
>> job search
>> http://corp.mail.com/careers
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
>> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: commons-logging & classloading (continued)
Posted by robert burrell donkin <ro...@blueyonder.co.uk>.
On 4 Nov 2003, at 13:44, Will Jaynes wrote:
<snip>
> I don't think that what I am asking is unreasonable. If the
> Log.isAssignableFrom(logClass) predicate fails then c-l should try
> again. Here is another suggested code change. It keeps your design and
> only does something more as a last resort.
>
> if (!Log.class.isAssignableFrom(logClass)) {
> // Plan B. Bend over backwards to continue
> Class logInterface =
> LoadClass("org.apache.commons.logging.Log");
> if (logInterface == null) {
> throw new LogConfigurationException
> ("Log interface can not be found");
> }
> if (!logInterface.isAssignableFrom(logClass))
> throw new LogConfigurationException
> ("Class " + logClassName + " does not implement Log");
> }
>
>
> Please consider these suggestions, or at least consider the spirit of
> these suggestions, which is to make commons-logging as correct,
> friendly and usable as possible.
i think i'm in favour of a change along these lines both for Log and
LogFactory. backward compatibility will be preserved but though i agree
that really people should set up things to avoid this, i think that
logging implementations should throw as few exceptions as possible. so,
unless someone else speaks up with a good reasons not to, i'll take a
look at this sometime soonish.
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: commons-logging & classloading (continued)
Posted by Will Jaynes <ja...@umich.edu>.
Remy Maucherat wrote:
> Will Jaynes wrote:
>
>> Well, I submitted this as a bug and included a patch. But Remy
>> immediately marked it as RESOLVED/WONTFIX. He doesn't really address
>> my suggestion other than to say that c-l "if used properly ... is well
>> thought out, and works perfectly well" I would never suggest that c-l
>> is not well thought out. However, I don't think that all the problems
>> we see involving c-l classloading can be attributed to us not using
>> c-l "properly". It may work perfectly well with Tomcat5, but that
>> doesn't mean there aren't improvements that can be made. Or even
>> accomodations that could be made to other containers than Tomcat5.
>>
>> Could someone please explain why the suggestion to load the Log
>> interface with the loadClass() method is not acceptable.
>
>
> - The core Log classes should be loaded once from one place (=
> (basically) they should be in the system classloader)
> - the log implementations (ex: the log4j implementation) should reside
> in the same classloader as the logger; ideally, the logger ships with
> wrapper classes for commons-logging
>
> This is the most efficient design, and fairly easy to understand.
> Coincidentally, this is the design that commons-logging implements, and
> it is good for a complex container environment.
>
> Remy
Look, Remy, you are right. The current design is the right one. And my
attempt at a refactoring unnecessarily changed that fundamental design.
If there were a perfect world where all containers were perfect and
acted just like Tomcat5 then I wouldn't have any problem and wouldn't
want a refactoring. But not all containers implement classloading just
as Tomcat5 does. And many of us have no choice but to live with those
containers. Because commons-logging is and is going to be ubiquitous it
has a responsility not only to be right but to be accomodating. If it
tries the right thing and fails, before it throws an Exception and quits
it should bend over backwards to try and find a way to work. It isn't
right that my whole application should fail just because my container
doesn't act exactly the way commons-logging thinks it should.
I don't think that what I am asking is unreasonable. If the
Log.isAssignableFrom(logClass) predicate fails then c-l should try
again. Here is another suggested code change. It keeps your design and
only does something more as a last resort.
if (!Log.class.isAssignableFrom(logClass)) {
// Plan B. Bend over backwards to continue
Class logInterface =
LoadClass("org.apache.commons.logging.Log");
if (logInterface == null) {
throw new LogConfigurationException
("Log interface can not be found");
}
if (!logInterface.isAssignableFrom(logClass))
throw new LogConfigurationException
("Class " + logClassName + " does not implement Log");
}
Please consider these suggestions, or at least consider the spirit of
these suggestions, which is to make commons-logging as correct, friendly
and usable as possible.
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: commons-logging & classloading (continued)
Posted by Remy Maucherat <re...@apache.org>.
Will Jaynes wrote:
> Well, I submitted this as a bug and included a patch. But Remy
> immediately marked it as RESOLVED/WONTFIX. He doesn't really address my
> suggestion other than to say that c-l "if used properly ... is well
> thought out, and works perfectly well" I would never suggest that c-l is
> not well thought out. However, I don't think that all the problems we
> see involving c-l classloading can be attributed to us not using c-l
> "properly". It may work perfectly well with Tomcat5, but that doesn't
> mean there aren't improvements that can be made. Or even accomodations
> that could be made to other containers than Tomcat5.
>
> Could someone please explain why the suggestion to load the Log
> interface with the loadClass() method is not acceptable.
- The core Log classes should be loaded once from one place (=
(basically) they should be in the system classloader)
- the log implementations (ex: the log4j implementation) should reside
in the same classloader as the logger; ideally, the logger ships with
wrapper classes for commons-logging
This is the most efficient design, and fairly easy to understand.
Coincidentally, this is the design that commons-logging implements, and
it is good for a complex container environment.
Remy
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: commons-logging & classloading (continued)
Posted by Will Jaynes <ja...@umich.edu>.
Will Jaynes wrote:
>
>
> robert burrell donkin wrote:
> ...
>
>> 2, a more sophisticated implementation for the loading of the
>> LogFactory implementation that would try to load the class from the
>> LogFactory class classloader if the assignment fails. since (at the
>> moment) we throw an exception in this circumstance (from would be very
>> difficult to recover from), i don't think that add this extra step
>> would do any harm.
>>
>> comments anyone?
>
>
> I agree with Norbert that the central remaining classloading problem is
> that the Log interface may not have been loaded by the same classloader
> as the log implementation, so the isAssignableFrom test fails. To be
> consistent, the Log interface should be loaded by the loadClass()
> method, just as the logClass is. Here's my suggested code change from
> LogFactoryImpl.getLogConstructor():
>
> // Attempt to load the Log implementation class
> Class logClass = null;
> Class logInterface = null;
> try {
> logClass = loadClass(logClassName);
> logInterface = loadClass("org.apache.commons.logging.Log");
> if (logClass == null) {
> throw new LogConfigurationException
> ("No suitable Log implementation for " + logClassName);
> }
> if (logInterface == null) {
> throw new LogConfigurationException
> ("Log interface not found by classloader");
> }
> if (!logInterface.isAssignableFrom(logClass)) {
> throw new LogConfigurationException
> ("Class " + logClassName + " does not implement Log");
> }
> } catch (Throwable t) {
> throw new LogConfigurationException(t);
> }
>
> It is certainly possible to also add Robert's suggestion of trying the
> LogFactory class classloader if the initial load fails, but I don't
> think that is necessary.
>
> Will
Well, I submitted this as a bug and included a patch. But Remy
immediately marked it as RESOLVED/WONTFIX. He doesn't really address my
suggestion other than to say that c-l "if used properly ... is well
thought out, and works perfectly well" I would never suggest that c-l is
not well thought out. However, I don't think that all the problems we
see involving c-l classloading can be attributed to us not using c-l
"properly". It may work perfectly well with Tomcat5, but that doesn't
mean there aren't improvements that can be made. Or even accomodations
that could be made to other containers than Tomcat5.
Could someone please explain why the suggestion to load the Log
interface with the loadClass() method is not acceptable.
Will
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: commons-logging & classloading (continued)
Posted by Will Jaynes <ja...@umich.edu>.
robert burrell donkin wrote:
...
> 2, a more sophisticated implementation for the loading of the LogFactory
> implementation that would try to load the class from the LogFactory
> class classloader if the assignment fails. since (at the moment) we
> throw an exception in this circumstance (from would be very difficult to
> recover from), i don't think that add this extra step would do any harm.
>
> comments anyone?
I agree with Norbert that the central remaining classloading problem is
that the Log interface may not have been loaded by the same classloader
as the log implementation, so the isAssignableFrom test fails. To be
consistent, the Log interface should be loaded by the loadClass()
method, just as the logClass is. Here's my suggested code change from
LogFactoryImpl.getLogConstructor():
// Attempt to load the Log implementation class
Class logClass = null;
Class logInterface = null;
try {
logClass = loadClass(logClassName);
logInterface = loadClass("org.apache.commons.logging.Log");
if (logClass == null) {
throw new LogConfigurationException
("No suitable Log implementation for " + logClassName);
}
if (logInterface == null) {
throw new LogConfigurationException
("Log interface not found by classloader");
}
if (!logInterface.isAssignableFrom(logClass)) {
throw new LogConfigurationException
("Class " + logClassName + " does not implement Log");
}
} catch (Throwable t) {
throw new LogConfigurationException(t);
}
It is certainly possible to also add Robert's suggestion of trying the
LogFactory class classloader if the initial load fails, but I don't
think that is necessary.
Will
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org