Request for Review (XS): 7143760 Memory leak in GarbageCollectionNotifications

David Holmes david.holmes at oracle.com
Mon Feb 13 22:58:43 PST 2012


Hi Fred,

On 13/02/2012 8:11 PM, Frederic Parain wrote:
> Here's the new webrev:
> http://cr.openjdk.java.net/~fparain/7143760/webrev.02/
>
> The changes are:
> - fix typo in comments
> - roll-back wrong THREAD->CHECK modifications
> - add a wrapper catching exceptions around the method
> sending notifications in order to prevent premature
> termination of the service thread.

My only concern with this is that I don't see anything in the 
javax.management docs describing the threading model for notifications 
and listeners, or for how exceptions from listeners are processed. 
Developers may assume that uncaught exception processing will occur if 
their listeners encounter exceptions, whereas the implementation 
"catches" all exceptions and ignores them. I guess this is more a JMX 
spec issue.

So thumbs up from me.

David
-----

> Thanks,
>
> Fred
>
> On 02/13/12 03:56 AM, David Holmes wrote:
>> Hi Fred,
>>
>> 182 class NotificationMark : public StackObj {
>>
>> Really we should have a general purpose utility class that can serve in
>> this role. This is the second time in a couple of weeks that the need to
>> deal with cleanup with CHECK has been uncovered.
>>
>> Not saying you necessarily need to do it for this CR.
>>
>> 183 // This class is used in GCNitifer::sendNotification to ensure
>>
>> Typo: nitifer :)
>>
>> 203 Handle objGcInfo =
>> createGcInfo(request->gcManager,request->gcStatInfo, CHECK);
>>
>> 212 instanceOop gc_mbean =
>> request->gcManager->get_memory_manager_instance(CHECK);
>>
>> CHECK should only be added to functions that can cause exceptions to
>> become pending.
>>
>> That all said I'm not sure that this fix hasn't gone the wrong way. If
>> sendNotification generates an exception then the serviceThread will
>> terminate. Is that the desired behaviour? Other event processing can't
>> terminate the service thread.
>>
>> David
>> -----
>>
>>
>> On 10/02/2012 11:04 PM, Frederic Parain wrote:
>>> Here's a new webrev addressing the following issues:
>>> - the missing HandleMark
>>> - the clean up of the GCNotificationRequest instance
>>> - removal of the pending exception testing, now
>>> exception will be propagated as soon as a method
>>> returns with a pending exception
>>>
>>> http://cr.openjdk.java.net/~fparain/7143760/webrev.01/
>>>
>>> Thanks,
>>>
>>> Fred
>>>
>>> On 2/10/12 11:27 AM, David Holmes wrote:
>>>> On 10/02/2012 7:59 PM, Dmitry Samersoff wrote:
>>>>> Frederic,
>>>>>
>>>>> GCNotificationRequest *request = getRequest();
>>>>>
>>>>> request variable also leaks memory because it will never be deleted on
>>>>> CHECK return path. Could you fix it too?
>>>>
>>>> Further:
>>>>
>>>> 211 JavaCalls::call_virtual(&result,
>>>> 212 gc_mbean_klass,
>>>> 213 vmSymbols::createGCNotification_name(),
>>>> 214 vmSymbols::createGCNotification_signature(),
>>>> 215 &args,
>>>> 216 CHECK);
>>>> 217 if (HAS_PENDING_EXCEPTION) {
>>>> 218 CLEAR_PENDING_EXCEPTION;
>>>> 219 }
>>>> 220
>>>> 221 delete request;
>>>>
>>>> The CHECK at @216 will cause a return if there is an exception pending
>>>> so 217-219 is dead code. This also indicates some confusion about what
>>>> exceptions this method can leave pending. Or it may be that the
>>>> CHECK at
>>>> #216 was meant to be just THREAD. ??
>>>>
>>>> (Strange this is the second example I've seen of this today!)
>>>>
>>>> David
>>>>
>>>>>
>>>>> -Dmitry
>>>>>
>>>>>
>>>>> On 2012-02-10 13:27, Frederic Parain wrote:
>>>>>> Here's a small fix (one line) for CR 7143760 Memory leak in
>>>>>> GarbageCollectionNotifications
>>>>>>
>>>>>> There's a missing HandleMark at the beginning of the
>>>>>> GCNotifier::sendNotificatin() method. Without this HandleMark, all
>>>>>> handles used when creating GC notifications are kept alive causing a
>>>>>> double leak: in the Java heap and in the thread local handle area of
>>>>>> the
>>>>>> service thread.
>>>>>>
>>>>>> Here's the CR:
>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7143760
>>>>>> (Warning, the changeset referenced in the CR is not the
>>>>>> one containing the original bug).
>>>>>>
>>>>>> Here's the webrev:
>>>>>> http://cr.openjdk.java.net/~fparain/7143760/webrev.00/
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Fred
>>>>>>
>>>>>
>>>>>
>>>
>


More information about the serviceability-dev mailing list