commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Simon Kitching <s_kitch...@yahoo.com>
Subject Re: [logging, beanutils, others] Initial proposal for java.lang.ContextClassLoaderLocal or java.lang.Singleton
Date Wed, 23 Mar 2005 14:08:37 GMT
Phil Steitz <phil.steitz <at> gmail.com> writes:

> 
> Thanks, Simon.  Much clearer now.   

Well, that´s unfortunate, because I think I completely stuffed up the 
explanation :-(. I wrote that in an airport travel lounge after a 10-hour 
flight and in retrospect should have waited until my mind was clearer. The info 
wasn´t exactly wrong, but didn´t address the bigger picture.

So sorry for the previous post, and this is what I think I should have said the 
first time:

Library code may wish to use singletons. And that library may be used in either 
stand-alone apps or deployed within a container. The obvious singleton 
implementation (using a static field on a class) works fine for:
 (a) standalone apps that don´t mess with classloaders
 (b) components deployed within containers where child-first classloading
     is done and all the necessary libs are bundled with the deployed component.

Personally, I think (b) should be *compulsory* for all container frameworks, 
and that we should simply declare that anyone who uses a commons library in an 
environment that breaks that rule will get what they deserve.

However there *are* a number of major containers which *do* break this rule by 
default. IBM Websphere is one; Resin is another. Even Tomcat provides an 
*option* to do parent-first classloading (though it´s not the default) and 
Tomcat documentation appears to encourage people to put jars in the shared/lib 
dir.

And violating (b) makes the Singleton instance shared across all the components 
deployed within the container rather than having a separate instance per 
(supposedly independent) deployed component. As concrete examples, logging 
config done by one deployed component can suddenly affect the logging of all 
components. And custom ConvertUtils converters registered by one component 
suddenly are visible to all components. This behaviour is almost certainly 
incorrect and unexpected to the user. And unfortunately it may also have quite 
subtle effects that would be very nasty indeed to debug; things that only show 
up when two cmoponents deployed together are accessed in certain patterns. 
Worse still, a user of commons-logging for example may not really be aware that 
there is a Singleton being used under the covers (to store the concrete 
LogFactory object in this case).

All this is of course because we are assuming a naive implementation of 
Singleton: using a static member on a Class. So BeanUtils and JCL tried a more 
sophisticated Singleton implementation: a static Map keyed by context 
classloader. This resolves all of the problems above, providing a separate 
Singleton instance per deployed component in a container while still working 
fine in non-container situations. But it introduced a new bug: a memory leak on 
component undeploy. My previous email describes the issues and potential 
solutions associated with this.

I also think the map-keyed-by-classloader approach is an inelegant hack anyway. 
It is slower, more complex and less memory-efficient than the static-member 
approach.


> 
> <snip/> 
> 
> > No, I mean the situation where a container continues running, while a webapp
> > or EJB running within it is stopped & restarted (potentially with updated
> > code).
> 
> OK,  so then lifecycle events should get triggered.

Yes, it should be possible for the author of a "component" to trap undeploy and 
perform explicit cleanup for libraries that use the static-map-keyed-by-context-
classloader approach currently used by beanutils and JCL.

> 
> <snip/>
> > 
>  > 
> > Commons provides libraries that might be used in j2ee environments, or might
> > be used in stand-alone apps; we don't know. So we need to write code that
> > works well in both environments.
> 
> Agreed, but I am still resisting taking more responsibility than just
> exposing APIs that support cleanup.

The problems with this are:
 * Users might not be aware that cleanup is needed. We can only express 
   this via documentation that people might or might not read.
 * I think it quite likely that library developers could add a singleton
   and forget to document it prominently
 * How many singletons is it sane to expect a j2ee component developer
   to clean up after? Two? Five? Ten?

Think about the j2ee apps you´ve written. Did you read the documentation for 
each and every library you used to see whether there was an explicit cleanup 
that needed to be done on undeploy?

I agree with you that this approach is *possible*. I just think it´s a shame 
it´s necessary. We all moved to java because we didn´t want to have to remember 
to free each piece of allocated memory; here we´re reintroducing the same kind 
of programmer burden.

An attempt has been made to fix this (in both beanutils and jcl) using weak 
references, but it´s only a partial fix. I personally think that this fix is 
*not* a good idea to include in a release. That´s a shame as it does fix the 
problem in some cases. However documenting what cases it does and doesn´t work 
in in a way that joe programmer can understand will be very nasty.

My recommendations therefore would be one of these (best first):
 * rip out all the Singleton workaround stuff currently in JCL and
   beanutils in favour of a plain static-member implementation, and
   document that we only support the "sane" child-first classloading
   approach.
 * keep the current code, but don´t do any weak-reference stuff. Instead,
   document that if people use the stupid parent-first approach and 
   put singleton-using libs in the shared classloader then they MUST
   use component life-cycle methods to catch undeploy and call the
   cleanup methods for each library as necessary [which is what I
   think you were recommending anyway].
 * create a commons-singleton library that uses probing and custom
   strategies etc to figure out how to correctly store singletons
   in the given runtime environment.

I think proposing a Singleton or ContextClassLoaderLocal class in the java core 
is also useful; having this as part of the language will give people an option 
of correctly implementing singletons even when using the bizarre parent-first 
classloading approach. However even if accepted it won´t be an option for JCL 
or beanutils or any other commons project for many years.

> > 
> > Currently, when JCL 1.0.4 is used in a j2ee environment *and* commons-
> > logging.jar is deployed in a shared classloader the application (or the
> > container) needs to add a call to LogManager.release when the component is
> > stopped/undeployed. This is a bit of a nuisance. It is even more of a 
nuisance
> > if a developer hasn't actually wanted to use JCL directly, and it is there
> > only because they use something like commons-foobar.jar which depends upon
> > commons-logging. Suddenly they need to "know" to add this extra call into
> > their app on undeploy. It would be really nice if commons-logging would
> > just "do the right thing" without putting extra burden on the developer.
> 
> Maybe now beaten into submission, but still wanting to respond that
> [foobar] should be aware of resources that it is allocating and expose
> its own cleanup API, which the user invokes to clean up everything
> that [foobar] has allocated, including releasing loggers, etc (which
> the [foobar] user, I agree, not have to worry about).

You´re saying that if I write commons-foobar that depends on jcl but doesn´t 
otherwise need cleanup, that I should provide a cleanup method that would 
invoke JCL´s cleanup method? Reasonable suggestion, though if the author of 
commons-foobar was focussing on stand-alone usage they might well forget that 
JCL (or whatever) needs cleaning up in container environments.

> > 
> > If there were simply a way to create a Singleton object that would work for
> > stand-alone java apps *and also* work on a per-component basis in a j2ee
> > framework that would solve the problem. But there doesn't seem to be a way 
to
> > write a Singleton correctly without asking the *user* of a library to
> > explicitly know about all the singletons that the library uses internally 
and
> > to clean them up on component stop/undeploy.
> >
> But to make this work in general, we need to make sure that the
> lifecycle of whatever backs the singleton (in your proposed solution,
> IIUC a context classloader) coincides exactly  with the lifecycle of
> all its users.  I guess the vague spec passage that Robert quotes here
> http://jakarta.apache.org/commons/logging/tech.html#J2EE%20Context%
20Classloaders
> means that this should work for J2EE components.

You´re saying that some containers might not have a per-component classloader, 
or set the context-classloader before executing code associated with the 
component?

Well, in the first case they really can´t expect "isolation" of components from 
each other then; singletons are clearly expected to be shared across all 
components in such a setup so there is no issue.

In the second case, I guess it´s possible that some containers might not use 
context classloaders. The j2ee spec (though vague) and the javadoc for 
Thread.setContextClassLoader do imply though that this is expected of 
containers. And it just plain seems *sensible* to me that a context classloader 
indicates a "scope" boundary. Worth debating though....

> 
> Like I said, I am not questioning the value of the "ClassLoaderLocal"
> concept or, at the end of the day, the practicality of using this kind
> of thing in [logging] and [beanutils] (or other commons components
> that need singletons), just trying to understand better exactly what
> it means and the pros and cons of depending on it.  Thanks for the
> clarification.

Well, I hope my "reclarification" is more useful...

Cheers,

Simon




---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org


Mime
View raw message