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.hol...@oracle.com
<mailto:david.hol...@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
