On Sun, 2006-02-19 at 20:27 +0100, Manfred Geiler wrote:
> On 2/19/06, Simon Kitching <[EMAIL PROTECTED]> wrote:
> > Just a warning to all developers: when using commons-logging in a
> > library, STATIC fields must **NOT** be used to store Log objects.
> >
> > The problem is that the class may be called with various thread-context
> > classloaders (TCCL) set. However the class is only ever loaded once.
>
> Never heard of that. Simon are you really sure?
> I think the opposite is true. Containers tend to have Classloaders
> that all have there own set of loaded classes. So you get problems
> when your applications Classloader holds some Classes that the
> container itself has already loaded. Then you get such things like
> "incomptible class" Exceptions and NoClassDefFound when a class in
> your webapp is linked to another class in your app but the Loader
> finds the containers class first.
Yes, I'm really sure.
Here's the relevant paragraph from the user-guide for the upcoming 1.1
release [written by me]:
http://jakarta.apache.org/commons/logging/guide.html#Developing%20With%
20JCL
This is not about class compatibility issues; using instance members
rather than class members doesn't change the classpath/classloader/etc.
It's about the fact that when the field is static, there is only *one*
Log instance for that class but multiple webapps are using it. Once
constructed, the Log object simply points directly to a *configured*
object within the underlying concrete logging library. That underlying
object therefore either:
(a) has no webapp-specific state (ie is initialised via config info
associated with the container not a TCCL)
(b) has the state of the first webapp that initialised it (ie is
initialised via config info located via the TCCL active when the Log
object was created).
Option (a) occurs if the first call to the class is from the container
itself, or if the concrete logging library used is in a "shared"
classpath, or if TCCL-based configuration is not enabled in
commons-logging. In this case, using a static Log is actually ok.
Option (b) occurs when TCCL-based configuration is enabled, and a webapp
is the first to access that class.
The latter case is the usual one, resulting in logging output generated
*as a result of webapp B calling MyFaces* ending up in the logfiles for
*webapp A*. Not good.
Any logging library that attempts to direct output from shared classes
into the logfile of the webapp *it is running on behalf of* will have
this issue. Log4j doesn't have this problem by default, as it doesn't
attempt to put output from shared classes into webapp-specific logfiles.
However it *does* have an optional mechanism to do this: the
RepositorySelector.
http://www.qos.ch/logging/sc.jsp
If the "Contextual Repository Selector" approach is used as documented
in the above page, then exactly the same benefits and problems occur in
log4j as in commons-logging; logging by shared classes goes into the
relevant webapp logfile [good] but static Log objects in shared classes
can output to the wrong logfile [bad].
Avoiding static Log instances in potentially shared classes is the only
reliable solution.
And as I mentioned in the earlier email, static fields in shared classes
should be regarded with great suspicion in *all* cases, not just
logging!
Simon,
Could you do me a favor and publicize this in the Struts community as well? The framework code there is littered with static log instances to. You might also want to add some notes related to using Log instances in Serializable classes (see below).
MyFaces folks,
There *is* a JSF-specific consideration to think about, if you have classes that implement StateHolder (like a UIComponent implementation). Log instances will generally *not* be serializable, so you will need to deal specially with them in saveState() and restoreState() methods. The simplest thing is to just not save them, and do a "new" operation again in restoreState().
Along the same lines, if your class implements Serializable, you will need to mark the instance variable transient. I've started using the following pattern in my Serializable classes, which would work inside a StateHolder as well:
private transient Log log = null;
private Log log() {
if (log == null) {
log = LogFactory.getLog(...);
}
return log;
}
and a typical call looks like:
if (log().isDebugEnabled()) {
log().debug("...");
}
Regards,
Simon
Craig
