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

[Daniel D. Daugherty]

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

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