Memory Leak at WSDLManagerImpl

[bharath-5]

I figured out a memory leak at WSDLManagerImpl schemaCacheMap.
The schemaCacheMap here has a weak key - WSDLDefinition and value
ServiceSchemaInfo. A key,value pair is inserted into this map while building
a service. The entry is never explicitly removed from this map. Since the
map is a WeakHashMap, it is assumed that when the key WSDLDefinition is
weakly referenced, the entry would be removed from the map. But it is seen
that the value ServiceSchemaInfo(the value) holds on to a SchemaInfo which
in turn holds on to the ServiceInfo, which holds the WSDLDefinition through
the AbstractPropertiesHolder.
This would mean that the value of the CacheMap always strongly refers to the
key, which would mean the entry would never be removed from the map.
"*The value objects in a WeakHashMap are held by ordinary strong references.
Thus care should be taken to ensure that value objects do not strongly refer
to their own keys, either directly or indirectly, since that will prevent
the keys from being discarded"

*This would mean ServiceInfo, OperationInfo, BindingInfo, WSDLDefinition
would all stay in the VM even after the endpoint is stopped.

One solution I could think of was to explicitly remove the entry on endpoint
stop, instead of relying on the WeakHashMap semantics.  This would work fine
for server endpoints. But Is there any way to do so for client endpoints?

[dkulp]

Hmmm....   good sleuthing.

You definitely don't want to unregister it on endpoint stop or when  
done with the client.   The whole point of the cache is to hold onto  
it so it's reusable.  On popular thing to do is create a client, use  
it once, discard.  Create another client, use it once, discard.  
Etc...  The cache is to help that.

I think what might make some sense is to get rid of the schemaCacheMap  
entirely.   Change the definitionsMap to be a Map<Object, DefPair> map  
where DepPair holds a wsdldef and the schema.   Thus, the keys would  
be the same keys for the wsdl.

The put/set's would be more expensive as you would need to iterate  
through the cache to find the pair.  I doubt the map would be very big  
though so that may be acceptable.

Dan



On May 31, 2008, at 4:55 PM, Bharath Ganesh wrote:

> I figured out a memory leak at WSDLManagerImpl schemaCacheMap.
> The schemaCacheMap here has a weak key - WSDLDefinition and value
> ServiceSchemaInfo. A key,value pair is inserted into this map while  
> building
> a service. The entry is never explicitly removed from this map.  
> Since the
> map is a WeakHashMap, it is assumed that when the key WSDLDefinition  
> is
> weakly referenced, the entry would be removed from the map. But it  
> is seen
> that the value ServiceSchemaInfo(the value) holds on to a SchemaInfo  
> which
> in turn holds on to the ServiceInfo, which holds the WSDLDefinition  
> through
> the AbstractPropertiesHolder.
> This would mean that the value of the CacheMap always strongly  
> refers to the
> key, which would mean the entry would never be removed from the map.
> "*The value objects in a WeakHashMap are held by ordinary strong  
> references.
> Thus care should be taken to ensure that value objects do not  
> strongly refer
> to their own keys, either directly or indirectly, since that will  
> prevent
> the keys from being discarded"
>
> *This would mean ServiceInfo, OperationInfo, BindingInfo,  
> WSDLDefinition
> would all stay in the VM even after the endpoint is stopped.
>
> One solution I could think of was to explicitly remove the entry on  
> endpoint
> stop, instead of relying on the WeakHashMap semantics.  This would  
> work fine
> for server endpoints. But Is there any way to do so for client  
> endpoints?

---
Daniel Kulp
dkulp@...
http://www.dankulp.com/blog

[Bharath Ganesh-2]

Yes we would need to iterate through the map while putting/getting the
ServiceSchemaInfo.
Do you think we should instead choose to retain the schemaCacheMap, but make
the value always a weak object. (Putting a WeakReference of the
ServiceSchemaInfo in the map). This way also we can solve the current
problem.
What do you say?



On Sun, Jun 1, 2008 at 7:19 AM, Daniel Kulp <dkulp@...> wrote:

>
> Hmmm....   good sleuthing.
>
> You definitely don't want to unregister it on endpoint stop or when done
> with the client.   The whole point of the cache is to hold onto it so it's
> reusable.  On popular thing to do is create a client, use it once, discard.
>  Create another client, use it once, discard.  Etc...  The cache is to help
> that.
>
> I think what might make some sense is to get rid of the schemaCacheMap
> entirely.   Change the definitionsMap to be a Map<Object, DefPair> map where
> DepPair holds a wsdldef and the schema.   Thus, the keys would be the same
> keys for the wsdl.
>
> The put/set's would be more expensive as you would need to iterate through
> the cache to find the pair.  I doubt the map would be very big though so
> that may be acceptable.
>
> Dan
>
>
>
>
> On May 31, 2008, at 4:55 PM, Bharath Ganesh wrote:
>
>  I figured out a memory leak at WSDLManagerImpl schemaCacheMap.
>> The schemaCacheMap here has a weak key - WSDLDefinition and value
>> ServiceSchemaInfo. A key,value pair is inserted into this map while
>> building
>> a service. The entry is never explicitly removed from this map. Since the
>> map is a WeakHashMap, it is assumed that when the key WSDLDefinition is
>> weakly referenced, the entry would be removed from the map. But it is seen
>> that the value ServiceSchemaInfo(the value) holds on to a SchemaInfo which
>> in turn holds on to the ServiceInfo, which holds the WSDLDefinition
>> through
>> the AbstractPropertiesHolder.
>> This would mean that the value of the CacheMap always strongly refers to
>> the
>> key, which would mean the entry would never be removed from the map.
>> "*The value objects in a WeakHashMap are held by ordinary strong
>> references.
>> Thus care should be taken to ensure that value objects do not strongly
>> refer
>> to their own keys, either directly or indirectly, since that will prevent
>> the keys from being discarded"
>>
>> *This would mean ServiceInfo, OperationInfo, BindingInfo, WSDLDefinition
>> would all stay in the VM even after the endpoint is stopped.
>>
>> One solution I could think of was to explicitly remove the entry on
>> endpoint
>> stop, instead of relying on the WeakHashMap semantics.  This would work
>> fine
>> for server endpoints. But Is there any way to do so for client endpoints?
>>
>
> ---
> Daniel Kulp
> dkulp@...
> http://www.dankulp.com/blog
>
>
>
>
>

[bharath-5]

On a related note -
There seems to be one more interesting issue here at WSDLManagerImpl. The
definitionsMap holds the WSDLDefinitions against a weak key, again relying
on the WeakHashMap semantics for removal.

The loadDefinition(String) method loads the WSDLDef and puts this in a map
against a String key. But this String key, is a literal String and will be
present in the constant pool, where garbage collection never happens. This
would mean the key would always be referenced from the constant pool, and
the entry would never be removed:-(

We shall fix this by always putting a URL as key in this map. The only valid
usage I saw was from WSDLServiceFactory. The WSDLServiceFactory, instead of
using the  wsdlManager.getDefinition(String) method, can create a URL from
this String, strongly refer the URL from wsdlURL field, and invoke the
wsdlManager.getDefinition(URL) method.

Any suggestions?


On Sun, Jun 1, 2008 at 2:25 AM, Bharath Ganesh <bharath@...> wrote:

> I figured out a memory leak at WSDLManagerImpl schemaCacheMap.
> The schemaCacheMap here has a weak key - WSDLDefinition and value
> ServiceSchemaInfo. A key,value pair is inserted into this map while building
> a service. The entry is never explicitly removed from this map. Since the
> map is a WeakHashMap, it is assumed that when the key WSDLDefinition is
> weakly referenced, the entry would be removed from the map. But it is seen
> that the value ServiceSchemaInfo(the value) holds on to a SchemaInfo which
> in turn holds on to the ServiceInfo, which holds the WSDLDefinition through
> the AbstractPropertiesHolder.
> This would mean that the value of the CacheMap always strongly refers to
> the key, which would mean the entry would never be removed from the map.
> "*The value objects in a WeakHashMap are held by ordinary strong
> references. Thus care should be taken to ensure that value objects do not
> strongly refer to their own keys, either directly or indirectly, since that
> will prevent the keys from being discarded"
>
> *This would mean ServiceInfo, OperationInfo, BindingInfo, WSDLDefinition
> would all stay in the VM even after the endpoint is stopped.
>
> One solution I could think of was to explicitly remove the entry on
> endpoint stop, instead of relying on the WeakHashMap semantics.  This would
> work fine for server endpoints. But Is there any way to do so for client
> endpoints?
>

[dkulp]

On the client side, I specifically made sure the call to set the wsdl  
location on the factories did a "location = new String(location);"  
type thing to make sure the String is not an interned string.   Had to  
do that due to the strings being compiled directly in to the code and  
such.   It's quite possible that hasn't been done on the server side,  
but probably should.

It's good that you're doing this.   The way the "standalone" tck  
works, we don't hit these things.   We have that setup to create a new  
Bus (and thus new WSDLManagerImpl) for each of the deployed  
application.  Also, everything is deployed upfront at startup.   They  
aren't ever undeployed/redeployed.

Dan


On Jun 2, 2008, at 4:44 AM, Bharath Ganesh wrote:

> On a related note -
> There seems to be one more interesting issue here at  
> WSDLManagerImpl. The
> definitionsMap holds the WSDLDefinitions against a weak key, again  
> relying
> on the WeakHashMap semantics for removal.
>
> The loadDefinition(String) method loads the WSDLDef and puts this in  
> a map
> against a String key. But this String key, is a literal String and  
> will be
> present in the constant pool, where garbage collection never  
> happens. This
> would mean the key would always be referenced from the constant  
> pool, and
> the entry would never be removed:-(
>
> We shall fix this by always putting a URL as key in this map. The  
> only valid
> usage I saw was from WSDLServiceFactory. The WSDLServiceFactory,  
> instead of
> using the  wsdlManager.getDefinition(String) method, can create a  
> URL from
> this String, strongly refer the URL from wsdlURL field, and invoke the
> wsdlManager.getDefinition(URL) method.
>
> Any suggestions?
>
>
> On Sun, Jun 1, 2008 at 2:25 AM, Bharath Ganesh <bharath@...>  
> wrote:
>
>> I figured out a memory leak at WSDLManagerImpl schemaCacheMap.
>> The schemaCacheMap here has a weak key - WSDLDefinition and value
>> ServiceSchemaInfo. A key,value pair is inserted into this map while  
>> building
>> a service. The entry is never explicitly removed from this map.  
>> Since the
>> map is a WeakHashMap, it is assumed that when the key  
>> WSDLDefinition is
>> weakly referenced, the entry would be removed from the map. But it  
>> is seen
>> that the value ServiceSchemaInfo(the value) holds on to a  
>> SchemaInfo which
>> in turn holds on to the ServiceInfo, which holds the WSDLDefinition  
>> through
>> the AbstractPropertiesHolder.
>> This would mean that the value of the CacheMap always strongly  
>> refers to
>> the key, which would mean the entry would never be removed from the  
>> map.
>> "*The value objects in a WeakHashMap are held by ordinary strong
>> references. Thus care should be taken to ensure that value objects  
>> do not
>> strongly refer to their own keys, either directly or indirectly,  
>> since that
>> will prevent the keys from being discarded"
>>
>> *This would mean ServiceInfo, OperationInfo, BindingInfo,  
>> WSDLDefinition
>> would all stay in the VM even after the endpoint is stopped.
>>
>> One solution I could think of was to explicitly remove the entry on
>> endpoint stop, instead of relying on the WeakHashMap semantics.  
>> This would
>> work fine for server endpoints. But Is there any way to do so for  
>> client
>> endpoints?
>>

---
Daniel Kulp
dkulp@...
http://www.dankulp.com/blog

[Bharath Ganesh-2]

This has not been on the server side - The value of the wsdlLocation
attribute (from @WebService annotation) is passed down under to the
WSDLServiceFactory, which obviously will be a literal.

On Mon, Jun 2, 2008 at 5:13 PM, Daniel Kulp <dkulp@...> wrote:

>
> On the client side, I specifically made sure the call to set the wsdl
> location on the factories did a "location = new String(location);" type
> thing to make sure the String is not an interned string.   Had to do that
> due to the strings being compiled directly in to the code and such.   It's
> quite possible that hasn't been done on the server side, but probably
> should.
>
> It's good that you're doing this.   The way the "standalone" tck works, we
> don't hit these things.   We have that setup to create a new Bus (and thus
> new WSDLManagerImpl) for each of the deployed application.  Also, everything
> is deployed upfront at startup.   They aren't ever undeployed/redeployed.
>
> Dan
>
>
>
> On Jun 2, 2008, at 4:44 AM, Bharath Ganesh wrote:
>
>  On a related note -
>> There seems to be one more interesting issue here at WSDLManagerImpl. The
>> definitionsMap holds the WSDLDefinitions against a weak key, again relying
>> on the WeakHashMap semantics for removal.
>>
>> The loadDefinition(String) method loads the WSDLDef and puts this in a map
>> against a String key. But this String key, is a literal String and will be
>> present in the constant pool, where garbage collection never happens. This
>> would mean the key would always be referenced from the constant pool, and
>> the entry would never be removed:-(
>>
>> We shall fix this by always putting a URL as key in this map. The only
>> valid
>> usage I saw was from WSDLServiceFactory. The WSDLServiceFactory, instead
>> of
>> using the  wsdlManager.getDefinition(String) method, can create a URL from
>> this String, strongly refer the URL from wsdlURL field, and invoke the
>> wsdlManager.getDefinition(URL) method.
>>
>> Any suggestions?
>>
>>
>> On Sun, Jun 1, 2008 at 2:25 AM, Bharath Ganesh <bharath@...>
>> wrote:
>>
>>  I figured out a memory leak at WSDLManagerImpl schemaCacheMap.
>>> The schemaCacheMap here has a weak key - WSDLDefinition and value
>>> ServiceSchemaInfo. A key,value pair is inserted into this map while
>>> building
>>> a service. The entry is never explicitly removed from this map. Since the
>>> map is a WeakHashMap, it is assumed that when the key WSDLDefinition is
>>> weakly referenced, the entry would be removed from the map. But it is
>>> seen
>>> that the value ServiceSchemaInfo(the value) holds on to a SchemaInfo
>>> which
>>> in turn holds on to the ServiceInfo, which holds the WSDLDefinition
>>> through
>>> the AbstractPropertiesHolder.
>>> This would mean that the value of the CacheMap always strongly refers to
>>> the key, which would mean the entry would never be removed from the map.
>>> "*The value objects in a WeakHashMap are held by ordinary strong
>>> references. Thus care should be taken to ensure that value objects do not
>>> strongly refer to their own keys, either directly or indirectly, since
>>> that
>>> will prevent the keys from being discarded"
>>>
>>> *This would mean ServiceInfo, OperationInfo, BindingInfo, WSDLDefinition
>>> would all stay in the VM even after the endpoint is stopped.
>>>
>>> One solution I could think of was to explicitly remove the entry on
>>> endpoint stop, instead of relying on the WeakHashMap semantics.  This
>>> would
>>> work fine for server endpoints. But Is there any way to do so for client
>>> endpoints?
>>>
>>>
> ---
> Daniel Kulp
> dkulp@...
> http://www.dankulp.com/blog
>
>
>
>
>

[Bharath Ganesh-2]

I have verified the GC with the below fix(schemaCacheMap has a weak value)
and the literal String fix(now always use the url at WSDLServiceFactory as
key).

After an application undeployment, things look fine on the Java EE Server.
Except that the entry in JAXBCONTEXT_CACHE in JAXBDataBinding would be still
retained till the next endpoint deployment/undeployment - WeakHashMap
expunges stale entries only on map access. Thus the classloader and JAXB
classes of the undeployed application would still be on the heap till the
next app deployment. This is not a memory leak. Later we can think of a
daemon thread which will access this map periodically to force stale entry
expunging.

Will commit these changes on 2.1 branch. DanK hope you will apply on 2.0.7
also.


On Mon, Jun 2, 2008 at 11:55 AM, Bharath Ganesh <bharathganesh@...>
wrote:

> Yes we would need to iterate through the map while putting/getting the
> ServiceSchemaInfo.
> Do you think we should instead choose to retain the schemaCacheMap, but
> make the value always a weak object. (Putting a WeakReference of the
> ServiceSchemaInfo in the map). This way also we can solve the current
> problem.
> What do you say?
>
>
>
> On Sun, Jun 1, 2008 at 7:19 AM, Daniel Kulp <dkulp@...> wrote:
>
>>
>> Hmmm....   good sleuthing.
>>
>> You definitely don't want to unregister it on endpoint stop or when done
>> with the client.   The whole point of the cache is to hold onto it so it's
>> reusable.  On popular thing to do is create a client, use it once, discard.
>>  Create another client, use it once, discard.  Etc...  The cache is to help
>> that.
>>
>> I think what might make some sense is to get rid of the schemaCacheMap
>> entirely.   Change the definitionsMap to be a Map<Object, DefPair> map where
>> DepPair holds a wsdldef and the schema.   Thus, the keys would be the same
>> keys for the wsdl.
>>
>> The put/set's would be more expensive as you would need to iterate through
>> the cache to find the pair.  I doubt the map would be very big though so
>> that may be acceptable.
>>
>> Dan
>>
>>
>>
>>
>> On May 31, 2008, at 4:55 PM, Bharath Ganesh wrote:
>>
>>  I figured out a memory leak at WSDLManagerImpl schemaCacheMap.
>>> The schemaCacheMap here has a weak key - WSDLDefinition and value
>>> ServiceSchemaInfo. A key,value pair is inserted into this map while
>>> building
>>> a service. The entry is never explicitly removed from this map. Since the
>>> map is a WeakHashMap, it is assumed that when the key WSDLDefinition is
>>> weakly referenced, the entry would be removed from the map. But it is
>>> seen
>>> that the value ServiceSchemaInfo(the value) holds on to a SchemaInfo
>>> which
>>> in turn holds on to the ServiceInfo, which holds the WSDLDefinition
>>> through
>>> the AbstractPropertiesHolder.
>>> This would mean that the value of the CacheMap always strongly refers to
>>> the
>>> key, which would mean the entry would never be removed from the map.
>>> "*The value objects in a WeakHashMap are held by ordinary strong
>>> references.
>>> Thus care should be taken to ensure that value objects do not strongly
>>> refer
>>> to their own keys, either directly or indirectly, since that will prevent
>>> the keys from being discarded"
>>>
>>> *This would mean ServiceInfo, OperationInfo, BindingInfo, WSDLDefinition
>>> would all stay in the VM even after the endpoint is stopped.
>>>
>>> One solution I could think of was to explicitly remove the entry on
>>> endpoint
>>> stop, instead of relying on the WeakHashMap semantics.  This would work
>>> fine
>>> for server endpoints. But Is there any way to do so for client endpoints?
>>>
>>
>> ---
>> Daniel Kulp
>> dkulp@...
>> http://www.dankulp.com/blog
>>
>>
>>
>>
>>
>

[dkulp]

On Jun 2, 2008, at 9:17 AM, Bharath Ganesh wrote:

> I have verified the GC with the below fix(schemaCacheMap has a weak  
> value)
> and the literal String fix(now always use the url at  
> WSDLServiceFactory as
> key).
>
> After an application undeployment, things look fine on the Java EE  
> Server.
> Except that the entry in JAXBCONTEXT_CACHE in JAXBDataBinding would  
> be still
> retained till the next endpoint deployment/undeployment - WeakHashMap
> expunges stale entries only on map access. Thus the classloader and  
> JAXB
> classes of the undeployed application would still be on the heap  
> till the
> next app deployment. This is not a memory leak. Later we can think  
> of a
> daemon thread which will access this map periodically to force stale  
> entry
> expunging.
>
> Will commit these changes on 2.1 branch. DanK hope you will apply on  
> 2.0.7
> also.

Definitely.  Once they are there on 2.1, I'll port them over to  
2.0.7.   Great work.

Dan

>
>
>
> On Mon, Jun 2, 2008 at 11:55 AM, Bharath Ganesh <bharathganesh@...
> >
> wrote:
>
>
>> Yes we would need to iterate through the map while putting/getting  
>> the
>> ServiceSchemaInfo.
>> Do you think we should instead choose to retain the schemaCacheMap,  
>> but
>> make the value always a weak object. (Putting a WeakReference of the
>> ServiceSchemaInfo in the map). This way also we can solve the current
>> problem.
>> What do you say?
>>
>>
>>
>> On Sun, Jun 1, 2008 at 7:19 AM, Daniel Kulp <dkulp@...> wrote:
>>
>>>
>>> Hmmm....   good sleuthing.
>>>
>>> You definitely don't want to unregister it on endpoint stop or  
>>> when done
>>> with the client.   The whole point of the cache is to hold onto it  
>>> so it's
>>> reusable.  On popular thing to do is create a client, use it once,  
>>> discard.
>>> Create another client, use it once, discard.  Etc...  The cache is  
>>> to help
>>> that.
>>>
>>> I think what might make some sense is to get rid of the  
>>> schemaCacheMap
>>> entirely.   Change the definitionsMap to be a Map<Object, DefPair>  
>>> map where
>>> DepPair holds a wsdldef and the schema.   Thus, the keys would be  
>>> the same
>>> keys for the wsdl.
>>>
>>> The put/set's would be more expensive as you would need to iterate  
>>> through
>>> the cache to find the pair.  I doubt the map would be very big  
>>> though so
>>> that may be acceptable.
>>>
>>> Dan
>>>
>>>
>>>
>>>
>>> On May 31, 2008, at 4:55 PM, Bharath Ganesh wrote:
>>>
>>> I figured out a memory leak at WSDLManagerImpl schemaCacheMap.
>>>> The schemaCacheMap here has a weak key - WSDLDefinition and value
>>>> ServiceSchemaInfo. A key,value pair is inserted into this map while
>>>> building
>>>> a service. The entry is never explicitly removed from this map.  
>>>> Since the
>>>> map is a WeakHashMap, it is assumed that when the key  
>>>> WSDLDefinition is
>>>> weakly referenced, the entry would be removed from the map. But  
>>>> it is
>>>> seen
>>>> that the value ServiceSchemaInfo(the value) holds on to a  
>>>> SchemaInfo
>>>> which
>>>> in turn holds on to the ServiceInfo, which holds the WSDLDefinition
>>>> through
>>>> the AbstractPropertiesHolder.
>>>> This would mean that the value of the CacheMap always strongly  
>>>> refers to
>>>> the
>>>> key, which would mean the entry would never be removed from the  
>>>> map.
>>>> "*The value objects in a WeakHashMap are held by ordinary strong
>>>> references.
>>>> Thus care should be taken to ensure that value objects do not  
>>>> strongly
>>>> refer
>>>> to their own keys, either directly or indirectly, since that will  
>>>> prevent
>>>> the keys from being discarded"
>>>>
>>>> *This would mean ServiceInfo, OperationInfo, BindingInfo,  
>>>> WSDLDefinition
>>>> would all stay in the VM even after the endpoint is stopped.
>>>>
>>>> One solution I could think of was to explicitly remove the entry on
>>>> endpoint
>>>> stop, instead of relying on the WeakHashMap semantics.  This  
>>>> would work
>>>> fine
>>>> for server endpoints. But Is there any way to do so for client  
>>>> endpoints?
>>>>
>>>
>>> ---
>>> Daniel Kulp
>>> dkulp@...
>>> http://www.dankulp.com/blog
>>>
>>>
>>>
>>>
>>>
>>

---
Daniel Kulp
dkulp@...
http://www.dankulp.com/blog