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

[David Holmes]

On 13/02/2012 3:58 PM, Krystal Mok wrote:
> 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");
> }

To clarify what I was saying, the actual service thread logic does:

     if (has_jvmti_events) {
       jvmti_event.post();
     }

     if (sensors_changed) {
       LowMemoryDetector::process_sensor_changes(jt);
     }

     if(has_gc_notification_event) {
         GCNotifier::sendNotification(CHECK);
     }

So only the GC notification events can trigger its termination due to 
the exception. If this includes propagating exceptions from listener 
code then this behaviour definitely seems incorrect to me.

David
-----

> 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
> <mailto: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
>
>
>
>
>