[10] RFR for JDK-8169961: Memory leak after debugging session

[Shafi Ahmad]

Hi Daniel,

Thank you for the review.

> -----Original Message-----
> From: Daniel D. Daugherty
> Sent: Thursday, July 13, 2017 7:40 PM
> To: Shafi Ahmad <shafi.s.ahmad at oracle.com>; serviceability-
> dev at openjdk.java.net
> Subject: Re: [10] RFR for JDK-8169961: Memory leak after debugging session
> 
> On 7/13/17 2:45 AM, Shafi Ahmad wrote:
> > Hi,
> >
> > Please review the code change for the fix of bug 'JDK-8169961: Memory
> leak after debugging session'
> >
> > Summary:
> > 1.  It seems that the thread created for
> com.sun.tools.jdi.TargetVM.EventController is never stopped and keeps a
> hard  reference to the VirtualMachineImpl. This leads to VirtualMachineImpl
> leak after debug session is finished.  EventController is private class and
> member field vm of type  VirtualMachineImpl, holding the hard reference to
> the VirtualMachineImpl. I am not seeing the usage of filed vm so we can
> remove it safely.
> > 2. Added eventController.release(); before  'Target VM interface thread
> exiting'
> > 3. TargetVM gets an EventController  which is a daemon thread, but don't
> see the thread having a way of stopping so added code to exit as soon as
> TargetVM thread stops listening.
> >
> > jdk10 bug: https://bugs.openjdk.java.net/browse/JDK-8169961
> > webrev link: http://cr.openjdk.java.net/~shshahma/8169961/webrev.00/
> 
> src/jdk.jdi/share/classes/com/sun/tools/jdi/TargetVM.java
>      L330:         VirtualMachineImpl vm;
>      L335:             this.vm = vm;
>          This comment caught my eye:
>          > I am not seeing the usage of filed vm so we can remove it safely.
> 
>          So you're deleting the 'vm' instance variable, but:
> 
>          L363: JDWP.VirtualMachine.HoldEvents.process(vm);
>          L365: JDWP.VirtualMachine.ReleaseEvents.process(vm);
> 
>          would use it if it was still there...
> 
>          Where did the run() method find a 'vm' variable? I'm guessing
>          the one in the TargetVM outer class:
> 
>          L46:     private VirtualMachineImpl vm;
> 
>          Okay so we don't have to pass (and save a copy of VirtualMachineImpl).
> 
>          So this constructor:
> 
>          L333:         EventController(VirtualMachineImpl vm) {
>              No longer uses the 'vm' parameter at all and you
>              can remove the parameter and update the caller.

332     private class EventController extends Thread {
333         int controlRequest = 0;
334 
335         EventController(VirtualMachineImpl vm) {
336             super(vm.threadGroupForJDI(), "JDI Event Control Thread");
337             setDaemon(true);
338             setPriority((MAX_PRIORITY + NORM_PRIORITY)/2);
339             super.start();
340         }

We can't remove formal parameter 'vm' as this is referenced at line# 336.

> 
> > Testing: run jprt and I provided the FBP and  it works for them.
> 
> JPRT doesn't execute any JDI related tests. You need to run the NSK JDI test
> suite and the com/sun/jdi tests in the JDK.

I will run NSK JDI test and update this thread with the results.

Regards,
Shafi

> 
> Dan
> 
> >
> > Regards,
> > Shafi
>