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

[Langer, Christoph]

Hi Shafi, Daniel,

In my initial review I oversaw the usage of vm inside the constructor. The new webrev looks even better. I guess the main fix is the termination of the thread anyway.

Best regards
Christoph

> -----Original Message-----
> From: serviceability-dev [mailto:serviceability-dev-
> bounces at openjdk.java.net] On Behalf Of Shafi Ahmad
> Sent: Montag, 17. Juli 2017 14:25
> To: Daniel Daugherty <daniel.daugherty at oracle.com>; serviceability-
> dev at openjdk.java.net
> Subject: RE: [10] RFR for JDK-8169961: Memory leak after debugging session
> 
> 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
> >