Re: [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.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...

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