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- > boun...@openjdk.java.net] On Behalf Of Shafi Ahmad > Sent: Montag, 17. Juli 2017 14:25 > To: Daniel Daugherty <daniel.daughe...@oracle.com>; serviceability- > d...@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.ah...@oracle.com>; serviceability- > > d...@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.ah...@oracle.com>; serviceability- > > >> d...@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 > >
