[10] RFR for JDK-8170299: Debugger does not stop inside the low memory notifications code [internal]

[David Holmes]

Hi Shafi,

I'm not at all sure about the approach you've taken with this ...

On 11/12/2017 8:31 PM, Shafi Ahmad wrote:
> Hi,
> 
> Could you please review the fix for JDK-8170299: Debugger does not stop 
> inside the low memory notifications code.
> 
> With the current implementation ‘debugger does not stop inside the low 
> memory *notifications* code’
> 
> This is expected as the notification code is getting executed with the 
> service thread, which is a hidden thread.

Okay. I recall some discussion on this problem - the main question being 
"why is the service thread executing user-defined Java code?". I'm 
assuming now this is an implementation artifact, not part of the main 
design of the management framework? But it is unclear. Attempting to 
introduce a new thread to execute the code may be reasonable, but raises 
  whole range of questions about concurrency, synchronization and 
possibly deadlock with this code!

> shafi at shafi-ahmad:~/Java/jdk8/jdk8u-dev$ jdb -Xmx32m MemoryWarningSystem
> 
> Initializing jdb ...
> 
>  > stop at MemoryWarningSystem$1:25
> 
> Deferring breakpoint MemoryWarningSystem$1:25.
> 
> It will be set after the class is loaded.
> 
>  > run
> 
> run MemoryWarningSystem
> 
> Set uncaught java.lang.Throwable
> 
> Set deferred uncaught java.lang.Throwable
> 
>  >
> 
> . . .
> 
>  > quit
> 
> Breakpoint not hit.
> 
> With the attached webrev, jdb stop  at breakpoint inside notification code.
> 
> shshahma at slc12kkg:/scratch/shshahma/Java/jdk-dev$ jdb -Xmx128m 
> MemoryWarningSystem
> 
> Initializing jdb ...
> 
>  > stop at MemoryWarningSystem$1:25
> 
> Deferring breakpoint MemoryWarningSystem$1:25.
> 
> It will be set after the class is loaded.
> 
>  > run
> 
> run MemoryWarningSystem
> 
> Set uncaught java.lang.Throwable
> 
> Set deferred uncaught java.lang.Throwable
> 
>  >
> 
> VM Started: Set deferred breakpoint MemoryWarningSystem$1:25
> 
> Breakpoint hit: "thread=Debugger", 
> MemoryWarningSystem$1.memoryUsageLow(), line=25 bci=0
> 
> 25            System.out.println("Memory usage low!!!");
> 
> Debugger[1] where
> 
>    [1] MemoryWarningSystem$1.memoryUsageLow (MemoryWarningSystem.java:25)
> 
>    [2] MemoryWarningSystem$2.handleNotification 
> (MemoryWarningSystem.java:48)
> 
>    [3] 
> javax.management.NotificationBroadcasterSupport.handleNotification 
> (NotificationBroadcasterSupport.java:284)
> 
>    [4] javax.management.NotificationBroadcasterSupport$SendNotifJob.run 
> (NotificationBroadcasterSupport.java:361)
> 
>    [5] java.util.concurrent.ThreadPoolExecutor.runWorker 
> (ThreadPoolExecutor.java:1,135)
> 
>    [6] java.util.concurrent.ThreadPoolExecutor$Worker.run 
> (ThreadPoolExecutor.java:635)
> 
>    [7] java.lang.Thread.run (Thread.java:844)
> 
>    [8] jdk.internal.misc.InnocuousThread.run (InnocuousThread.java:134)
> 
> Debugger[1] list
> 
> 21
> 
> 22        MemoryWarningSystem mws = new MemoryWarningSystem();
> 
> 23        mws.addListener(new MemoryWarningSystem.Listener() {
> 
> 24          public void memoryUsageLow(long usedMemory, long maxMemory) {
> 
> 25 =>         System.out.println("Memory usage low!!!");
> 
> 26            double percentageUsed = ((double) usedMemory) / maxMemory;
> 
> 27            System.out.println("percentageUsed = " + percentageUsed);
> 
> 28            MemoryWarningSystem.setPercentageUsageThreshold(0.8);
> 
> 29          }
> 
> 30        });
> 
> Debugger[1] threads
> 
> Group system:
> 
>    (java.lang.ref.Reference$ReferenceHandler)0x362 Reference Handler running
> 
>    (java.lang.ref.Finalizer$FinalizerThread)0x361  Finalizer         
> cond. waiting
> 
>    (java.lang.Thread)0x360                         Signal Dispatcher running
> 
> Group main:
> 
>    (java.lang.Thread)0x1                           main              running
> 
> Group InnocuousThreadGroup:
> 
>    (jdk.internal.misc.InnocuousThread)0x35f        Common-Cleaner    
> cond. waiting
> 
>    (jdk.internal.misc.InnocuousThread)0x4f1        Debugger   
>         running (at breakpoint)
> 
> Debugger[1] step
> 
>  > Memory usage low!!!
> 
> Step completed: "thread=Debugger", 
> MemoryWarningSystem$1.memoryUsageLow(), line=26 bci=8
> 
> 26            double percentageUsed = ((double) usedMemory) / maxMemory;
> 
> Please see the newly created thread “  
> (jdk.internal.misc.InnocuousThread)0x4f1        Debugger   
>         running (at breakpoint)”.
> 
> In the change, the class ‘MemoryImpl’ is derived from class 
> ‘NotificationBroadcasterSupport’ instead of class 
> ‘NotificationEmitterSupport’

That's quite a significant change and the implications of it are not at 
all obvious. The sun.management.NotificationEmitterSupport class 
utilises synchronization for thread-safety. The 
NotificationBroadcasterSupport class utilises a CopyOnWriteArraylist and 
will have quite different concurrency properties. Throw into the mix the 
fact you've now added a new thread and we really have no clear idea how 
this will all behave.

> NotificationBroadcastSupport takes an Executor, in the constructor of 
> MemoryImpl we are calling super() with an executor, which is a visible 
> thread.
> 
> The method hasListeners() is referenced  inside MemoryImpl.java and 
> defined in  NotificationEmitterSupport.
> 
> This method is not present in  NotificationBroadcasterSupport so I added 
> it to NotificationBroadcasterSupport.

You will need to file a CSR request first to add a public method to a 
public class. But it's not all clear to me this is a method that should 
be added to this API.

I see what you are trying to do, but the impact of these changes seem 
quite far reaching and hard to determine.

> Jdk10 bug: https://bugs.openjdk.java.net/browse/JDK-8170299

You should note that the bug is not publicly visible.

Thanks,
David

> Webrev link: http://cr.openjdk.java.net/~shshahma/8170299/webrev.02/
> 
> Testing: jprt -testset core, rbt --test nsk.jvmti.testlist,nsk.jdi.testlist.
> 
> Regards,
> 
> Shafi
>