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<ListenerInfo> 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
> <https://bugs.openjdk.java.net/browse/JDK-8170299>
> 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
> 
> 
>