[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: Friday, July 14, 2017 6:46 AM
> 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 10:19 AM, Shafi Ahmad wrote:
> > 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.
> 
> So the reference to 'vm' on L336 won't resolve to the one in the TargetVM
> outer class:
> 
> L46:     private VirtualMachineImpl vm;
> 
> like L363 and L365 do...

Yes, you are right as this is an inner class constructor. 

Please find updated webrev.
http://cr.openjdk.java.net/~shshahma/8169961/webrev.01/

Regards,
Shafi


> Dan
> 
> 
> >
> >>> 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
>