On 02/27/2009 01:15 PM, Bob Lee wrote:

On Fri, Feb 27, 2009 at 11:04 AM, David M. Lloyd <david.lloyd@redhat.com> wrote:

...

A couple use cases, off the top of my head:

1) I've got a set of FooBars that associate with Classes; now for whatever reason, I want to change the FooBar that is associated with the Class. The old FooBar is now completely unreferenced; keeping a reference to it is a leak.

What's a FooBar? Use cases should be concrete. :-)

...

2) I've got an application server deployment that provides some kind of service by class, so I stash refs on the ClassLoaders of the Classes for whom the service is provided. I want to undeploy my application, but all those classloaders have strong refs to my deployment. They all have to be cleared or you face a OOM: PermGen after a few redeployments. In this case I'd use a stop() + finalize() on my service which clears them.

Can you provide more detail? It sounds to me like you're saying that the service impl classes are in the parent class loader and the server (that binds the services) is in the child class loader, but from my experience, it's usually the other way around.

It comes back to the whole point of the exercise. Using JBoss Marshalling as a concrete use case - WeakHashMap<Class<?>, Externalizer>() *fails* because Externalizer instances are usually customized to the class they externalize (which, by the way, could well be a system class). This means that Externalizer keeps a strong ref to the Class after all. So we need a map with weak keys -> weak values, and a way to associate a strong ref from the Class. This covers the case of the Class going away. But if the deployment containing the Externalizer goes away? It's a leak, and a big one (it includes the whole classloader of the Externalizer implementation, not to mention the classloader of Externalizer.class itself) unless we can remove the reference somehow. Even if you attempt to work around it by using an intermediate object from the system classloader, like an Object[1], to hold the references, and you manually keep a Set of them and clear them all out on redeploy, you've still leaked an Object[1] on every class or classloader for every redeployment. So it doesn't matter what classloader the service is bound from, though it can exacerbate the problem. If I ever want to associate some data with, say, a class from the system classloader - *even if I make that data be a type from the system classloader* - that data is now permanent, even when I don't need it anymore. So if I cause a deployment to happen again, which re-executes the association, the old data is now leaked. Your solution puts permanent, uncollectible data on classloaders, no matter how you slice it. There *has* to be a way to clean it up. - DML

On 02/27/2009 02:18 PM, Bob Lee wrote:

On Fri, Feb 27, 2009 at 11:44 AM, David M. Lloyd <david.lloyd@redhat.com> wrote:

...

WeakHashMap<Class<?>, Externalizer>()

*fails* because Externalizer instances are usually customized to the class they externalize (which, by the way, could well be a system class). This means that Externalizer keeps a strong ref to the Class after all.

If the class is from a parent class loader, you should just keep a strong reference to the data and not use ClassLoader.keepReferenceTo().

I don't think you understood what I wrote - keeping a strong reference to the data defeats the purpose of the mechanism utterly. I'll try to lay it out a little more plainly - 1) I need to associate some data with classes - which can come from anywhere, any class loader. This is not an uncommon requirement. Anyone who has ever written a Map<Class<?>, ?> has realized this requirement. 2) The data may have a strong ref to the class it is associated with. This is a consequence of the purpose of associating data with a class key. 3) The module responsible for establishing the mapping must not maintain a strong reference to the class, to allow that classloader to be collected. 4) The module responsible for establishing the mapping must not maintain a strong reference to the data, to allow the deployment which created the data to be collected. 5) The mapping must remain until a collection of (3) or (4) occurs, or else the purpose of caching data per-class has been defeated. These rules would apply for *ANY* case where one might want to maintain a Map<Class<?>, ?> of any sort. Your solution allows the map, but does not allow the value to be changed or removed - EVER - and if the map is collected, all the values would still hang around. My solution fits the use cases I've provided. I don't understand how it could possibly be OK to *ever* put a permanent reference on to any classloader but your own, without a way to clean it up. And if you're going to do that, just use a static final field. I just don't see the use case that would benefit from such an ability. - DML

On 02/27/2009 02:59 PM, Bob Lee wrote:

On Fri, Feb 27, 2009 at 12:48 PM, David M. Lloyd <david.lloyd@redhat.com> wrote:

...

I don't think you understood what I wrote

I understood. I just think you're trying to solve a problem that no one really has. 99% of the time, the problem is with a class from a parent class loader keeping a strong reference to a class in a child class loader.

I'm not talking about a parent/child relationship at all. I'm talking about parent, child, AND sibling class loaders. You're presenting a very simplistic view here, but in a real application server, things can get a lot more complex. A class loader for JAR of an implementation of an API might not be visible to anyone else; however the API might be visible from several other deployments. And any deployment who has access to an API can pass data to any other deployment. It's not as simple as you make it out to be - there's not just two use cases. I think what you meant to say is "99% of the time (in Bob Lee's experience, and nobody else's experience matters here)..." You agree that there needs to be a mechanism to store a reference on a classloader. Yet you say that there is no valid use case for removing the reference. I've given you use cases. You reject them because you don't think they're important or common enough. Yet you do not produce a use case which *justifies* the ability to *add* a reference on to the classloader, which does not also cause a problem with leakage. I contend, unequivocally, that there is *no* justification to put a strong reference on any classloader but your own which you cannot again remove; and if you're going to do that, you should just use a static final field. You *must* either demonstrate the utility of such a situation (which, by the way, you just stated is no less than 99 times more common of a use case than the one I propose), or concede that reference removal is required. I don't see any other logical course. - DML

On 02/27/2009 03:51 PM, Bob Lee wrote:

There's no need for insults, David. Have some perspective. I've been nothing but civil and respectful (even after you presumed to know what I do and don't understand).

I haven't insulted you that I am aware of, only stated the facts as I see them. Since you haven't responded to any of my points, I can only assume that you do not understand them (which seems unlikely) or you're not interested in understanding (which does imply, among other things, a lack of respect). Being ignored in this way gives a distinct impression that you're not interested in any solution that did not spring from your own mind. Allow me to explain another way. You readily agreed that this problem needs a solution (after all, your presentation of your alternative solution was the start of the thread). But you stated flat out that removal should not be supported (no justification given); then you basically placed the burden on me to justify my position. Now I have no problem with that; I justified myself (more than one time, and quite concisely I thought). But you didn't respond to me at all - instead you circularly argued that my use case was invalid; I demonstrated that my use case could not be invalid without your use case also being invalid; you ignored me (twice) and restated that my use case was not valid without responding to my points. Does this help you understand my position?

Non-hierarchical class loaders fall under a) in my book--"code that should probably be redesigned."

But having used WebSphere in a past life, I realize that's too much to ask.

Not just websphere - any modern application server or container (think OSGi for another perspective on the same problem). This type of problem is also quite common with frameworks. Any application server which provides any level of isolation between deployments and libraries can and will give rise to a non-hierarchical classloader situation.

We should probably just wait for ephemerons. I think they alleviate the need for this altogether. With ephemerons, you could have a map of Class -> [some value] where [some value] doesn't prevent Class from being reclaimed. If Class if reclaimed, the reference to [some value] is dropped, too.

I like the notion of ephemerons, but unless I'm mistaken, a notion is all they are right now. I think that we could put in place a solution to this problem today, and then migrate the solution to ephemerons later on, if they ever become more than ephemeral. :-) - DML

David, I regret making my suggestion in the first place. I really think we need ephemerons, but for the sake of discussion: - Your patch adds 2 new classes. My suggestion adds one method (maybe 2 for convenience). - Your approach enables explicit clearing, but I thought the whole point of adding this extension was to avoid explicit clearing. If you're going to explicitly clear, why do you need this functionality at all? If we want to support circular dependencies between class loaders, we should pursue ephemerons because your solution requires explicit clearing whereas ephemerons would not. - Say for example that I need a static map from Class to List<Method> (some filtered list of methods in Class). Your patch requires one WeakHashMap per Class. My suggestion requires only one map total and one lightweight data structure per ClassLoader. - Your patch forces users to use your data structure. My suggestion enables users to use whatever data structure they like. Your patch introduces a point of contention between completely orthogonal libraries. Mine introduces almost none (assuming you implement the internal data structure in a non-locking fashion). Bob