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: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
