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
