openjpa-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marc Prud'hommeaux <mprud...@apache.org>
Subject Re: PCRegistry ClassLoader memory leak
Date Wed, 18 Jul 2007 23:34:59 GMT


> If the only time the WeakReference will be nullified is when the  
> class is no longer referenced, isn't it ok to use WeakReference?  
> What are the circumstances under which the class is not referenced?  
> If its class loader is garbage collected, or no class references  
> the pc class?
>
> The Registry need only keep track of currently used classes. If the  
> class loader decides to unload the class, it's ok to remove it from  
> the registry, as long as when the class loader needs it again it  
> initializes it.

The Meta.pc problem is with the *instance* of the class that is being  
kept around for pcNewInstance() purposes. If you have a WeakMap of  
SomeClass : Foo->Bar->InstanceOfSomeClass, then the SomeClass key can  
never be cleaned up. And if InstanceOfSomeClass is made weak, then it  
might be garbage collected.

> It's been a long time since this architecture was put into place  
> but I don't recall any real issues like these...

I submitted a bug report to Sun ("Incident Review ID: 261116") about  
this back in 2004. I believe there was also some discussion of it  
back in March of 2006 on the EG list.



On Jul 18, 2007, at 4:11 PM, Craig L Russell wrote:

>
> On Jul 18, 2007, at 3:51 PM, Marc Prud'hommeaux wrote:
>
>>
>> On Jul 18, 2007, at 2:52 PM, Kevan Miller wrote:
>>
>>>
>>> On Jul 17, 2007, at 5:32 PM, Marc Prud'hommeaux wrote:
>>>
>>>>
>>>> Correct me if I am wrong, but I think the problem is essentially  
>>>> that we have a Map of SomeClass:Object->Object->...->SomeClass,
 
>>>> and therefore SomeClass can never be garbage collected,  
>>>> regardless of whether the keys in the Map are soft or weak, and  
>>>> regardless of whether the class' ClassLoader is referenced  
>>>> elsewhere or not.
>>>>
>>>> Another possible solution might be to find every place where we  
>>>> have a Class field that could possibly be set to a  
>>>> PersistenceCapable, and turn them all into a  
>>>> WeakReference<Class>. I haven't checked to see how big a change  
>>>> that would be, but it might be one of the simpler solutions (and  
>>>> it might be faster than storing the class name as a String and  
>>>> doing a Class.forName() every time we need access to the Class  
>>>> object).
>>>
>>> There are two categories of references that are keeping  
>>> ClassLoaders alive.
>>>
>>> (1) References to classes (i.e. Meta fields pcClass, fieldTypes 
>>> [], and pcSuper). These can all be wrapped fairly easily using  
>>> WeakReferences. I've fixed these, locally.
>>
>> Sounds good.
>>
>>> (2) References to objects (i.e Meta.pc). This is a problem.  
>>> Meta.pc is the only reference to the PersistenceCapable object.  
>>> Since Meta.pc is the only reference to the PersistenceCapable  
>>> object, it can't be a WeakReference.
>
> If the only time the WeakReference will be nullified is when the  
> class is no longer referenced, isn't it ok to use WeakReference?  
> What are the circumstances under which the class is not referenced?  
> If its class loader is garbage collected, or no class references  
> the pc class?
>
> The Registry need only keep track of currently used classes. If the  
> class loader decides to unload the class, it's ok to remove it from  
> the registry, as long as when the class loader needs it again it  
> initializes it.
>
> It's been a long time since this architecture was put into place  
> but I don't recall any real issues like these...
>
> Craig
>>>
>>> Any thoughts on fixing the problem with Meta.pc? Does it need to  
>>> be an object? Can we generate an instance of PersistenceCapable  
>>> as needed?
>>
>> That is unfortunate. To my knowledge, the only reason we need to  
>> keep around an instance of the object is that it is easy to  
>> construct new instances using PersistenceCapable.pcNewInstance()  
>> without having to use reflection each time or worry about  
>> security. I'm not sure of any way around this.
>>
>>> Or, on a different tack, can PCRegistry be made non-static?  
>>> Associated with the PU, for instance?
>>
>> I personally think it would be nice if we could get rid of  
>> PCRegistry being static. However, this part of the problem with  
>> the Meta.pc issue in the previous paragraph: the static  
>> initializer of any enhanced persistent class will do:
>>
>>    PCRegistry.register(MyClass.class, pcFieldNames, pcFieldTypes,  
>> pcFieldFlags, pcPCSuperclass, "MyClass", new MyClass());
>>
>> If PCRegistry weren't static, then we wouldn't be able to do this.
>>
>> Also, I think there are a number of places where we rely on  
>> PCRegistry being static for other reasons. I think it would be  
>> nice to get rid of these, but that project might be too large a  
>> scope just to fix this bug.
>>
>>
>>> As Patrick mentions, another approach is to load OpenJPA in every  
>>> application classloader (or a unique OpenJPA ClassLoader per  
>>> application ClassLoader). I really don't want to do this... I'd  
>>> prefer a private interface to notify you of when ClassLoaders are  
>>> no longer valid...
>>>
>>> --kevan
>>>
>>>
>>>>
>>>>
>>>> On Jul 17, 2007, at 10:14 AM, Patrick Linskey wrote:
>>>>
>>>>> Of course, another approach is for Geronimo to trash the  
>>>>> classloader
>>>>> that OpenJPA was loaded in when the app is undeployed.
>>>>>
>>>>> -Patrick
>>>>>
>>>>> On 7/17/07, robert burrell donkin  
>>>>> <robertburrelldonkin@gmail.com> wrote:
>>>>>> On 7/17/07, Kevan Miller <kevan.miller@gmail.com> wrote:
>>>>>> >
>>>>>> > On Jul 17, 2007, at 12:15 AM, Craig L Russell wrote:
>>>>>> >
>>>>>> > >
>>>>>> > > On Jul 16, 2007, at 7:48 PM, Kevan Miller wrote:
>>>>>> > >
>>>>>> > >>
>>>>>> > >> On Jul 16, 2007, at 10:26 PM, Pinaki Poddar wrote:
>>>>>> > >>
>>>>>> > >>> Just to clarify:
>>>>>> > >>> I meant it should be the loader that loaded Person.class
 
>>>>>> where
>>>>>> > >>> Person is
>>>>>> > >>> the persistence capable class, *not*
>>>>>> > >>>  
>>>>>> org.apache.openjpa.enhance.PersistenceCapable.class.getClassLoade

>>>>>> r
>>>>>> > >>> ().
>>>>>> > >>>
>>>>>> > >>> But testing in multi-classloader environment is
required to
>>>>>> > >>> validate any
>>>>>> > >>> changes of this nature.
>>>>>> > >>
>>>>>> > >> Yes, that's how I interpreted your statement. My point
is  
>>>>>> that if
>>>>>> > >> SSN is a field of Person, the ClassLoader of SSN is
not
>>>>>> > >> necessarily the ClassLoader for Person. I'd be concerned
 
>>>>>> that the
>>>>>> > >> Person ClassLoader can't load SSN. In that case, your
 
>>>>>> technique
>>>>>> > >> wouldn't work...
>>>>>> > >
>>>>>> > > Just to clarify, if SSN is the type of a field of Person,
the
>>>>>> > > ClassLoader of Person.class must be able to load SSN  
>>>>>> (either itself
>>>>>> > > or via a parent ClassLoader) or you will have a linkage
 
>>>>>> error while
>>>>>> > > loading Person.
>>>>>> >
>>>>>> > Craig,
>>>>>> > You are correct. The declared type must be loadable. I was 

>>>>>> thinking
>>>>>> > of an SSNImpl type which need not be. However, that's not  
>>>>>> really
>>>>>> > relevant to the problem at hand...
>>>>>> >
>>>>>> > Pinaki,
>>>>>> > I'm afraid that I may have improperly narrowed your  
>>>>>> objectives by
>>>>>> > singling out fieldTypes. PCRegistry$Meta.pc and pcSuper  
>>>>>> could also
>>>>>> > keep Classes/ClassLoaders alive...
>>>>>>
>>>>>> if there are several places where this may happen then  
>>>>>> perhaps  try a
>>>>>> variant on http://svn.apache.org/repos/asf/jakarta/commons/ 
>>>>>> proper/logging/trunk/src/java/org/apache/commons/logging/impl/ 
>>>>>> WeakHashtable.java
>>>>>>
>>>>>> - robert
>>>>>>
>>>>>
>>>>>
>>>>> -- 
>>>>> Patrick Linskey
>>>>> 202 669 5907
>>>>
>>>
>>
>
> Craig Russell
> Architect, Sun Java Enterprise System http://java.sun.com/products/jdo
> 408 276-5638 mailto:Craig.Russell@sun.com
> P.S. A good JDO? O, Gasp!
>


Mime
View raw message