Re: RFR : JDK-8170299 - Debugger does not stop inside the low memory notifications code

[Daniel D. Daugherty]

Adding hotspot-runtime-dev@... to this thread...

More below...


On 8/28/18 3:37 PM, mandy chung wrote:



On 8/24/18 7:06 AM, Daniel Fuchs wrote:

Hi Harsha,

On 23/08/2018 17:35, Daniel Fuchs wrote:

So all in all - maybe this is worth fixing but better early
in the release than late. I also wonder whether such a behavior
change should deserve a release note (or a switch to revert
to the old behavior - though I do hope that isn't necessary).


On second thought - it occurred to me that what you are proposing
to do in the platform could very well be implemented in the
client (listener) code instead.

The workaround would be simple:

---
package com.foo.monitoring;

import javax.management.*;

final class PlatformMXBeaNotificationListener implements
  NotificationListener {

    private final NotoificationListener delegate;

    PlatformMXBeaNotificationListener(NotoificationListener delegate) {
this.delegate = delegate;
    }

    public void handleNotification(Notification notification,
   Object handback) {
 executor.execute(() ->
   delegate.handleNotification(notification, handback));
    }

    private static final Executor executor
  = Executors.newSingleTreadExecutor();
}


This way the threading would remain in control of the
client application, and the debugger would be able to
step in the delegate's handleNotification(...) method?

Given the unquantifiable risk posed by changing the threading
model in the platform, then maybe leaving the current
implementation as is and documenting that workaround
leaving it up to the client code to decide whether to use
that or not, would be the better idea?



I agree that this is a behavioral change but I suspect this is not 
high compatibility risk
though (no data though).   I understand David's concern that this is 
to fix a debugger
issue and we should investigate other alternatives to minimize the 
compatibility risk.


The other approach David has proposed is to investigate if we can make 
the service thread
not hidden.   The threading model in the notification listener 
invocation remains unchanged.
In addition, the VM service thread also handles DCMD and JVM TI 
callbacks.  I don't know
if there are public API to register DCMD callback.  If so, it will 
enable debugging
user-defined DCMD callback for free then.  The notion of hidden VM 
JavaThread was added
to avoid a compiler thread and other system thread being suspended and 
resumed
(see JDK-4529296).   So we should be careful and look into past issues 
to make the

VM thread visible to debugger.  It may be a feasible solution.

In any case, it would be useful to clarify the spec w.r.t. the 
synchronization and threads
invoking the listeners and users should prepare the listeners may be 
invoked

asynchronously in parallel.

Mandy


More work is being moved from the VMThread to the ServiceThread. I don't
think making the ServiceThread visible (and thus suspend-able) is going
to be viable.

JDK-8206424 Use ConcurrentHashTable for ProtectionDomainTable
https://bugs.openjdk.java.net/browse/JDK-8206424

JDK-8206423 Use ConcurrentHashTable for ResolvedMethodTable
https://bugs.openjdk.java.net/browse/JDK-8206423

There may be more work in progress for off loading the VMThread...

Dan

Re: RFR : JDK-8170299 - Debugger does not stop inside the low memory notifications code

[mandy chung]

On 8/24/18 7:06 AM, Daniel Fuchs wrote:

Hi Harsha,

On 23/08/2018 17:35, Daniel Fuchs wrote:

So all in all - maybe this is worth fixing but better early
in the release than late. I also wonder whether such a behavior
change should deserve a release note (or a switch to revert
to the old behavior - though I do hope that isn't necessary).


On second thought - it occurred to me that what you are proposing
to do in the platform could very well be implemented in the
client (listener) code instead.

The workaround would be simple:

---
package com.foo.monitoring;

import javax.management.*;

final class PlatformMXBeaNotificationListener implements
  NotificationListener {

    private final NotoificationListener delegate;

    PlatformMXBeaNotificationListener(NotoificationListener delegate) {
this.delegate = delegate;
    }

    public void handleNotification(Notification notification,
   Object handback) {
 executor.execute(() ->
   delegate.handleNotification(notification, handback));
    }

    private static final Executor executor
  = Executors.newSingleTreadExecutor();
}


This way the threading would remain in control of the
client application, and the debugger would be able to
step in the delegate's handleNotification(...) method?

Given the unquantifiable risk posed by changing the threading
model in the platform, then maybe leaving the current
implementation as is and documenting that workaround
leaving it up to the client code to decide whether to use
that or not, would be the better idea?



I agree that this is a behavioral change but I suspect this is not high 
compatibility risk
though (no data though).   I understand David's concern that this is to 
fix a debugger
issue and we should investigate other alternatives to minimize the 
compatibility risk.


The other approach David has proposed is to investigate if we can make 
the service thread
not hidden.   The threading model in the notification listener 
invocation remains unchanged.
In addition, the VM service thread also handles DCMD and JVM TI 
callbacks.  I don't know
if there are public API to register DCMD callback.  If so, it will 
enable debugging
user-defined DCMD callback for free then.  The notion of hidden VM 
JavaThread was added
to avoid a compiler thread and other system thread being suspended and 
resumed
(see JDK-4529296).   So we should be careful and look into past issues 
to make the

VM thread visible to debugger.  It may be a feasible solution.

In any case, it would be useful to clarify the spec w.r.t. the 
synchronization and threads
invoking the listeners and users should prepare the listeners may be 
invoked

asynchronously in parallel.

Mandy

Re: RFR : JDK-8170299 - Debugger does not stop inside the low memory notifications code

[Sundararajan Athijegannathan]

+1. It is better not to fix this bug.

-Sundar

On 24/08/18, 7:36 PM, Daniel Fuchs wrote:

Hi Harsha,

On 23/08/2018 17:35, Daniel Fuchs wrote:

So all in all - maybe this is worth fixing but better early
in the release than late. I also wonder whether such a behavior
change should deserve a release note (or a switch to revert
to the old behavior - though I do hope that isn't necessary).


On second thought - it occurred to me that what you are proposing
to do in the platform could very well be implemented in the
client (listener) code instead.

The workaround would be simple:

---
package com.foo.monitoring;

import javax.management.*;

final class PlatformMXBeaNotificationListener implements
  NotificationListener {

private final NotoificationListener delegate;

PlatformMXBeaNotificationListener(NotoificationListener delegate) {
this.delegate = delegate;
}

public void handleNotification(Notification notification,
   Object handback) {
 executor.execute(() ->
   delegate.handleNotification(notification, handback));
}

private static final Executor executor
  = Executors.newSingleTreadExecutor();
}


This way the threading would remain in control of the
client application, and the debugger would be able to
step in the delegate's handleNotification(...) method?

Given the unquantifiable risk posed by changing the threading
model in the platform, then maybe leaving the current
implementation as is and documenting that workaround
leaving it up to the client code to decide whether to use
that or not, would be the better idea?

best regards,

-- daniel

Re: RFR : JDK-8170299 - Debugger does not stop inside the low memory notifications code

[Daniel Fuchs]

Hi Harsha,

On a high level point of view, I think the fix looks good.
I like the use of CopyOnWriteArrayList and removeIf.
IIUC there is a single instance of ServiceThread in the VM, so
using a static single thread executor to call the listeners
should preserve the order in which the notifications are handled
without putting any additional strain on the VM - since you're
adding a single thread, which is lazily started when the
first listener needs to be invoked.

I see in the history of serviceabilty-dev that David was expressing
some concerns about the change of behaviour that offloading the
invocation of the listener to another thread would cause.

On the one hand I agree with that assertion: the new single thread
in which the listener are executed probably has a different
ACC etc... than the ServiceThread, and offloading to another
thread also could potentially invalidate assumptions that the
listeners code might have made. So this might not be transparent
to applications/libraries who might be using these notifications.
Also, unfortunately, would creating the new executor/thread
(+ runnable) become an issue when it happens at the time where
the memory pressure is already high (i.e. when the low memory
threshold is crossed)?

I'm not expert enough in the JVM to know whether there's any
JDK internal code (JFR?) that make use of these notification
listeners, and which might get tripped by this new threading.

On the other hand, offloading the invocation of the listener to
a dedicated thread could as well be safer, as previously a
badly implemented listener might have wedged the service
thread (which I assume would have been be bad). With your fix
it will only wedge the

So all in all - maybe this is worth fixing but better early
in the release than late. I also wonder whether such a behavior
change should deserve a release note (or a switch to revert
to the old behavior - though I do hope that isn't necessary).

Hopefully David & Mandy will chime in :-)


Now some small comments:

  79 boolean isPresent = listenerList.stream().anyMatch(li 
-> li.listener.equals(listener));


=> we're losing the atomicity here as a new listener might just
   have been added in between the call to listenerList.remove and
   this line, but I'm not sure it's important enough to fix.
   IMHO this new impl is good enough - even if it doesn't behave
   exactly as the old.


 102 NotificationListener listener;
 103 NotificationFilter filter;
 104 Object handback;

=> If I'm not mistaken these could (and therefore should) be final.

 134 if (filter != null ? !filter.equals(that.filter) : 
that.filter != null) return false;
 135 return handback != null ? 
handback.equals(that.handback) : that.handback == null;


=> Using Objects.equals(o1,o2) would make the code more readable here.

 141 result = 31 * result + (filter != null ? 
filter.hashCode() : 0);
 142 result = 31 * result + (handback != null ? 
handback.hashCode() : 0);


=> Similar to above Objects.hashCode(obj) would make the code
   simpler.

 149 Thread thread = InnocuousThread.newThread(runnable);

=> on a previous round of comment on this list Mandy
   had suggested using InnocuousThread.newSystemThread(runnable);

   I believe the main difference is in the TCCL, which is null
   for this latter case. Do we know what the TCCL of the
   ServiceThread is?

 151 thread.setName("JMX Notification thread");

=> I would suggest "Platform MXBean Notification thread" - "JMX"
   is too general.

 156 private List listenerList = new 
CopyOnWriteArrayList<>();


=> should be private *final*


hope this helps,

-- daniel

On 17/07/2018 07:23, Harsha Wardhana B wrote:

Hi All,

Please review the fix for the bug,

JDK-8170299 - Debugger does not stop inside the low memory notifications 
code


webrev at,

http://cr.openjdk.java.net/~hb/8170299/webrev.00/

Description of the fix:

The debugger does not stop inside the listeners registered for 
notification from


1. com.sun.management.GarbageCollectorMXBean 2. sun.management.MemoryImpl 
(MemoryMXBean)
3. com.sun.management.DiagnosticCommandMBean

The listeners registered for above MBeans are invoked by 'ServiceThread' which 
is a hidden thread and is not visible to the debugger.

This issue was was already worked on before and below is the review thread for 
the same.

http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021782.html
http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-December/022611.html

With the current fix, all the user registered callbacks for above MBeans 
are executed in a newly created SingleThreadExecutor. The above file is 
also re-factored to use CopyOnWriteArrayList for managing the listeners.


The fix has been tested in Mach5 by running all the tests under 
open/:jdk_management and closed/:jdk_management. 

RFR : JDK-8170299 - Debugger does not stop inside the low memory notifications code

[Harsha Wardhana B]

Hi All,

Please review the fix for the bug,

JDK-8170299 - Debugger does not stop inside the low memory notifications 
code


webrev at,

http://cr.openjdk.java.net/~hb/8170299/webrev.00/

Description of the fix:

The debugger does not stop inside the listeners registered for 
notification from


1. com.sun.management.GarbageCollectorMXBean 2. sun.management.MemoryImpl 
(MemoryMXBean)
3. com.sun.management.DiagnosticCommandMBean

The listeners registered for above MBeans are invoked by 'ServiceThread' which 
is a hidden thread and is not visible to the debugger.

This issue was was already worked on before and below is the review thread for 
the same.

http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021782.html
http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-December/022611.html

With the current fix, all the user registered callbacks for above MBeans 
are executed in a newly created SingleThreadExecutor. The above file is 
also re-factored to use CopyOnWriteArrayList for managing the listeners.


The fix has been tested in Mach5 by running all the tests under 
open/:jdk_management and closed/:jdk_management. The tests under 
open/test/jdk/java/lang/management/MemoryMXBean cover the above code 
changes. I can add more tests in the subsequent reviews if need arises.


Please review the above change and let me know your comments.

Thanks
Harsha