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

[Krystal Mok]

Hi all,

Tto follow up with what David mentioned:

If sendNotification generates an exception then the serviceThread will
> terminate. Is that the desired behaviour? Other event processing can't
> terminate the service thread.


I did a simple test to see what would happen in 7u2 when someone throws an
uncaught exception on the Service Thread [1]. And in this test it did make
the service thread terminate, because sendNotification() does:

try {
    li.listener.handleNotification(notification, li.handback);
} catch (Exception e) {
    e.printStackTrace();
    throw new InternalError("Error in invoking listener");
}

And that InternalError gets uncaught, which leads to the termination of the
thread. So regardless of what the VM does, it seems like the current
behavior of the Service Thread is to terminate whenever a handler throws an
exception. Don't know if that's the desired behavior, though.

- Kris

[1]:  https://gist.github.com/1814004

On Mon, Feb 13, 2012 at 10:56 AM, David Holmes <david.holmes at oracle.com>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/<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<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/<http://cr.openjdk.java.net/~fparain/7143760/webrev.00/>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Fred
>>>>>
>>>>>
>>>>
>>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20120213/30b5c99e/attachment.html