PCRegistry ClassLoader memory leak

[Kevan Miller]

Geronimo is running out of PermGen space in some simple deploy/
undeploy scenarios involving OpenJPA. The cause of the problem seems  
to be the _metas table in PCRegistry. _metas is a  
ConcurrentReferenceHashMap with WEAK reference keys and HARD  
reference values. The keys are the PersistenceCapable classes. While  
the values are the metadata for these classes which are maintained by  
the internal Meta class.

The cause of the ClassLoader memory leak is simple -- if any of the  
objects/classes held by the Meta class (e.g. fieldTypes) have also  
been loaded by the same ClassLoader used to load the  
PersistenceCapable class, the PersistenceCapable class (the weak key)  
will never be GCed. The value of the HashMap entry will always  
maintain a hard reference to the ClassLoader. Since the ClassLoader  
will never be GC'ed, the the the pcClass Class object will never be  
GC'able...

The problem can be easily recreated using current Geronimo trunk and  
the Geronimo Daytrader application.

--kevan

[Marc Prud'hommeaux]

Kevan-

How many deploy/undeploys does it take before it runs out of memory?

I recall a long time back we had a similar problem (although the  
error was entrenched in the same functionality contained in the  
javax.jdo.spi.JDOImplHelper. I don't know if a solution was ever  
found. Short of figuring out some way to listen for the death of a  
ClassLoader and manually unregistering metadata when that happens, I  
can't think of any way to deal with this automatically.

Anyone have any ideas?



On Jul 16, 2007, at 2:31 PM, Kevan Miller wrote:

> Geronimo is running out of PermGen space in some simple deploy/
> undeploy scenarios involving OpenJPA. The cause of the problem  
> seems to be the _metas table in PCRegistry. _metas is a  
> ConcurrentReferenceHashMap with WEAK reference keys and HARD  
> reference values. The keys are the PersistenceCapable classes.  
> While the values are the metadata for these classes which are  
> maintained by the internal Meta class.
>
> The cause of the ClassLoader memory leak is simple -- if any of the  
> objects/classes held by the Meta class (e.g. fieldTypes) have also  
> been loaded by the same ClassLoader used to load the  
> PersistenceCapable class, the PersistenceCapable class (the weak  
> key) will never be GCed. The value of the HashMap entry will always  
> maintain a hard reference to the ClassLoader. Since the ClassLoader  
> will never be GC'ed, the the the pcClass Class object will never be  
> GC'able...
>
> The problem can be easily recreated using current Geronimo trunk  
> and the Geronimo Daytrader application.
>
> --kevan

[Patrick Linskey-2]

> Anyone have any ideas?

Change fieldTypes to be Strings instead, and dematerialize them as needed.

-Patrick

On 7/16/07, Marc Prud'hommeaux <[hidden email]> wrote:

> Kevan-
>
> How many deploy/undeploys does it take before it runs out of memory?
>
> I recall a long time back we had a similar problem (although the
> error was entrenched in the same functionality contained in the
> javax.jdo.spi.JDOImplHelper. I don't know if a solution was ever
> found. Short of figuring out some way to listen for the death of a
> ClassLoader and manually unregistering metadata when that happens, I
> can't think of any way to deal with this automatically.
>
> Anyone have any ideas?
>
>
>
> On Jul 16, 2007, at 2:31 PM, Kevan Miller wrote:
>
> > Geronimo is running out of PermGen space in some simple deploy/
> > undeploy scenarios involving OpenJPA. The cause of the problem
> > seems to be the _metas table in PCRegistry. _metas is a
> > ConcurrentReferenceHashMap with WEAK reference keys and HARD
> > reference values. The keys are the PersistenceCapable classes.
> > While the values are the metadata for these classes which are
> > maintained by the internal Meta class.
> >
> > The cause of the ClassLoader memory leak is simple -- if any of the
> > objects/classes held by the Meta class (e.g. fieldTypes) have also
> > been loaded by the same ClassLoader used to load the
> > PersistenceCapable class, the PersistenceCapable class (the weak
> > key) will never be GCed. The value of the HashMap entry will always
> > maintain a hard reference to the ClassLoader. Since the ClassLoader
> > will never be GC'ed, the the the pcClass Class object will never be
> > GC'able...
> >
> > The problem can be easily recreated using current Geronimo trunk
> > and the Geronimo Daytrader application.
> >
> > --kevan
>
>

--
Patrick Linskey
202 669 5907

[robert burrell donkin-2]

On 7/16/07, Patrick Linskey <[hidden email]> wrote:
> > Anyone have any ideas?
>
> Change fieldTypes to be Strings instead, and dematerialize them as needed.

after spending hundreds of hours pursuing other solutions to a similar
issue, i that this would be wise.

<rant>
sounds very similar to the infamous commons logging leak. after a
*lot* of work, Brian Stansberry wrote a custom hashtable
implementation which (after a lot of testing and rewriting) didn't
seem to leak with modern JVMs. we used reference queues and periodic
purging.

IMO the real problem is that the J2EE specification lacks application
life cycle events. i've heard of other leaks (eg when using
reflection) which are hard or impossible to fix. of course, it doesn't
help that the classloader specifications are broke too...
</rant>

- robert

[Marc Prud'hommeaux]

I'm a little unclear about what "Change fieldTypes to be Strings  
instead" means. Do you mean the key in the map, or something?

One other solution would be to get rid of the Map and just use some  
sort of weak Set, but lookups would need to iterate through the set  
every time. I don't know what kind of performance hit this would incur.

The best solution would probably to just eliminate this (and all) non-
constant static fields. I remember trying very hard to do this some  
time ago, but I kept hitting walls preventing the complete  
elimination. Maybe smarter minds than me can figure it out, though.



On Jul 16, 2007, at 3:17 PM, robert burrell donkin wrote:

> On 7/16/07, Patrick Linskey <[hidden email]> wrote:
>> > Anyone have any ideas?
>>
>> Change fieldTypes to be Strings instead, and dematerialize them as  
>> needed.
>
> after spending hundreds of hours pursuing other solutions to a similar
> issue, i that this would be wise.
>
> <rant>
> sounds very similar to the infamous commons logging leak. after a
> *lot* of work, Brian Stansberry wrote a custom hashtable
> implementation which (after a lot of testing and rewriting) didn't
> seem to leak with modern JVMs. we used reference queues and periodic
> purging.
>
> IMO the real problem is that the J2EE specification lacks application
> life cycle events. i've heard of other leaks (eg when using
> reflection) which are hard or impossible to fix. of course, it doesn't
> help that the classloader specifications are broke too...
> </rant>
>
> - robert

[Pinaki Poddar-2]

Meta.fieldTypes is declared as Class[].
They are accessed via PCRegistry::getFieldTypes(Class pcClass).

The suggestion of "Change fieldTypes to be Strings instead" *perhaps*
meant to change Meta.fieldTypes to String[] (as field type names). And
only during
PCRegistry::getFieldTypes() 'lazily' converting the String[]s to
Class[]es (by the same ClassLoader of the PersistenceCapable class?).


 
 


Pinaki Poddar
972.834.2865

-----Original Message-----
From: Marc Prud'hommeaux [mailto:[hidden email]] On Behalf Of
Marc Prud'hommeaux
Sent: Monday, July 16, 2007 5:37 PM
To: [hidden email]
Subject: Re: PCRegistry ClassLoader memory leak


I'm a little unclear about what "Change fieldTypes to be Strings
instead" means. Do you mean the key in the map, or something?

One other solution would be to get rid of the Map and just use some sort
of weak Set, but lookups would need to iterate through the set every
time. I don't know what kind of performance hit this would incur.

The best solution would probably to just eliminate this (and all) non-
constant static fields. I remember trying very hard to do this some time
ago, but I kept hitting walls preventing the complete elimination. Maybe
smarter minds than me can figure it out, though.



On Jul 16, 2007, at 3:17 PM, robert burrell donkin wrote:

> On 7/16/07, Patrick Linskey <[hidden email]> wrote:
>> > Anyone have any ideas?
>>
>> Change fieldTypes to be Strings instead, and dematerialize them as
>> needed.
>
> after spending hundreds of hours pursuing other solutions to a similar

> issue, i that this would be wise.
>
> <rant>
> sounds very similar to the infamous commons logging leak. after a
> *lot* of work, Brian Stansberry wrote a custom hashtable
> implementation which (after a lot of testing and rewriting) didn't
> seem to leak with modern JVMs. we used reference queues and periodic
> purging.
>
> IMO the real problem is that the J2EE specification lacks application
> life cycle events. i've heard of other leaks (eg when using
> reflection) which are hard or impossible to fix. of course, it doesn't

> help that the classloader specifications are broke too...
> </rant>
>
> - robert


Notice:  This email message, together with any attachments, may contain information  of  BEA Systems,  Inc.,  its subsidiaries  and  affiliated entities,  that may be confidential,  proprietary,  copyrighted  and/or legally privileged, and is intended solely for the use of the individual or entity named in this message. If you are not the intended recipient, and have received this message in error, please immediately return this by email and then delete it.

[Kevan Miller]

On Jul 16, 2007, at 5:40 PM, Marc Prud'hommeaux wrote:

> Kevan-
>
> How many deploy/undeploys does it take before it runs out of memory?

Hi Marc,
It's application dependent. I think it took about 20 deploy/undeploy  
cycles. We're losing a ClassLoader per deploy/undeploy. Looks like  
for this app each ClassLoader is ~ 1 meg. I was running with an 80meg  
max permgen:

export JAVA_OPTS="-XX:MaxPermSize=80m -verbose:gc -XX:+PrintGCDetails  
-XX:+HeapDumpOnOutOfMemoryError"

FYI, here are the GC Roots for one of our ClassLoaders being leaked  
-- http://people.apache.org/~kevan/PCRegistryLeak.html

>
> I recall a long time back we had a similar problem (although the  
> error was entrenched in the same functionality contained in the  
> javax.jdo.spi.JDOImplHelper. I don't know if a solution was ever  
> found. Short of figuring out some way to listen for the death of a  
> ClassLoader and manually unregistering metadata when that happens,  
> I can't think of any way to deal with this automatically.
>
> Anyone have any ideas?

Well, with some increased complexity, you could have WeakReference  
values (have ConcurrentReferenceHashMap permit WeakReference keys and  
WeakReference values (or wrap Meta with a WeakReference before  
inserting into the _metas table).

Define an OpenJPA specific interface for triggering ClassLoader  
events. Either a callback interface or straight method call. The JSF  
specification defined a javax.faces.FactoryFinder.releaseFactories()  
method which can be called during an undeploy.  That's spec-defined  
however. I'd prefer not to go that way, but it's not impossible...

--kevan

[Kevan Miller]

On Jul 16, 2007, at 6:43 PM, Pinaki Poddar wrote:

> Meta.fieldTypes is declared as Class[].
> They are accessed via PCRegistry::getFieldTypes(Class pcClass).
>
> The suggestion of "Change fieldTypes to be Strings instead" *perhaps*
> meant to change Meta.fieldTypes to String[] (as field type names). And
> only during
> PCRegistry::getFieldTypes() 'lazily' converting the String[]s to
> Class[]es (by the same ClassLoader of the PersistenceCapable class?).

Right, you'll need a ClassLoader to load from. Defaulting the the CL  
of the PersistenceCapable class might work (for Geronimo), but  
doesn't seem right either...

--kevan

[Pinaki Poddar-2]

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.getClassLoader().

But testing in multi-classloader environment is required to validate any
changes of this nature.


Pinaki Poddar
972.834.2865

-----Original Message-----
From: Kevan Miller [mailto:[hidden email]]
Sent: Monday, July 16, 2007 9:22 PM
To: [hidden email]
Subject: Re: PCRegistry ClassLoader memory leak


On Jul 16, 2007, at 6:43 PM, Pinaki Poddar wrote:

> Meta.fieldTypes is declared as Class[].
> They are accessed via PCRegistry::getFieldTypes(Class pcClass).
>
> The suggestion of "Change fieldTypes to be Strings instead" *perhaps*
> meant to change Meta.fieldTypes to String[] (as field type names). And

> only during
> PCRegistry::getFieldTypes() 'lazily' converting the String[]s to
> Class[]es (by the same ClassLoader of the PersistenceCapable class?).

Right, you'll need a ClassLoader to load from. Defaulting the the CL of
the PersistenceCapable class might work (for Geronimo), but doesn't seem
right either...

--kevan

Notice:  This email message, together with any attachments, may contain information  of  BEA Systems,  Inc.,  its subsidiaries  and  affiliated entities,  that may be confidential,  proprietary,  copyrighted  and/or legally privileged, and is intended solely for the use of the individual or entity named in this message. If you are not the intended recipient, and have received this message in error, please immediately return this by email and then delete it.

[Kevan Miller]

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.getClassLoader().
>
> 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... Either  
way, would definitely require testing...

--kevan

[Craig L Russell]

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.getClassLoader().
>>
>> 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

> Either way, would definitely require testing...
>
> --kevan
>
>
>

Craig Russell
Architect, Sun Java Enterprise System http://java.sun.com/products/jdo
408 276-5638 mailto:[hidden email]
P.S. A good JDO? O, Gasp!

[Marina Vatkina]

I'm wondering if the EMF.close() either called by the user or by the container,
when the application is unloaded/undeployed can trigger the cleanup?

thanks,
-marina

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.getClassLoader().
>>>
>>> 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
>
>> Either way, would definitely require testing...
>>
>> --kevan
>>
>>
>>
>
> Craig Russell
> Architect, Sun Java Enterprise System http://java.sun.com/products/jdo
> 408 276-5638 mailto:[hidden email]
> P.S. A good JDO? O, Gasp!
>

[robert burrell donkin-2]

On 7/17/07, Marina Vatkina <[hidden email]> wrote:
> I'm wondering if the EMF.close() either called by the user or by the container,
> when the application is unloaded/undeployed can trigger the cleanup?

JEE unfortunately lacks lifecycle methods

- robert

[Kevan Miller]

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.getClassLoader
>>> ().
>>>
>>> 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...

--kevan

[robert burrell donkin-2]

On 7/17/07, Kevan Miller <[hidden email]> 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.getClassLoader
> >>> ().
> >>>
> >>> 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-2]

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 <[hidden email]> wrote:

> On 7/17/07, Kevan Miller <[hidden email]> 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.getClassLoader
> > >>> ().
> > >>>
> > >>> 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

[Pinaki Poddar-2]

 Kevan,
> 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...  

   I am afraid that I have spelt your name wrong in JIRA-285...

  The latest patch (JIRA-285.patch.2.txt) stringifies
PCRegistry$Meta.pcSuper also as per your observation.

  PCRegistry$Meta.pc, however, was declared as PersistenceCapable.
Tried stringifying the user-defined persistence capble class name and
recreate an instance in PCRegistry when required. The problem with that
approach is instantiation may not succeed because OpenJPA allows user
defined pc classes *not* to declare a public no-arg constructor. 6 Tests
in OpenJPA failed due to that restriction.
   I have currently left PCRegistry$Meta.pc as it was.


Pinaki Poddar
972.834.2865

-----Original Message-----
From: Kevan Miller [mailto:[hidden email]]
Sent: Tuesday, July 17, 2007 9:17 AM
To: [hidden email]
Subject: Re: PCRegistry ClassLoader memory leak


>
> On Jul 16, 2007, at 7:48 PM, Kevan Miller wrote:
>

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...

--kevan

Notice:  This email message, together with any attachments, may contain information  of  BEA Systems,  Inc.,  its subsidiaries  and  affiliated entities,  that may be confidential,  proprietary,  copyrighted  and/or legally privileged, and is intended solely for the use of the individual or entity named in this message. If you are not the intended recipient, and have received this message in error, please immediately return this by email and then delete it.

[Sahoo]

robert burrell donkin wrote:
> On 7/17/07, Marina Vatkina <[hidden email]> wrote:
>> I'm wondering if the EMF.close() either called by the user or by the
>> container,
>> when the application is unloaded/undeployed can trigger the cleanup?
>
> JEE unfortunately lacks lifecycle methods
>
Lifecycle of the container managed EMF is defined in the JPA spec in
section #7.1.1. Those rules don't apply for Java SE style of JPA in a
Java EE container. If that's a web app, then it should be able to use
something like *ServletContextListener.contextDestroyted*. I don't know
any such facilities for EJB container.

Thanks,
Sahoo

[Marc Prud'hommeaux]

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).


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 <[hidden email]>  
> wrote:
>> On 7/17/07, Kevan Miller <[hidden email]> 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.getClassLoader
>> > >>> ().
>> > >>>
>> > >>> 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

[Kevan Miller]

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.
(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.

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? Or, on a different tack, can PCRegistry be made non-static?  
Associated with the PU, for instance?

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 <[hidden email]>  
>> wrote:
>>> On 7/17/07, Kevan Miller <[hidden email]> 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.getClassLoader
>>> > >>> ().
>>> > >>>
>>> > >>> 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
>