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

[Shafi Ahmad]

Thank you Daniel and Serguei for reviewing it.

Please find updated webrev - I added volatile attribute to variable 'shouldListen'.

http://cr.openjdk.java.net/~shshahma/8169961/webrev.02/

Regards,
Shafi

> -----Original Message-----
> From: Serguei Spitsyn
> Sent: Thursday, August 10, 2017 12:27 AM
> To: Daniel Daugherty <daniel.daugherty at oracle.com>; Shafi Ahmad
> <shafi.s.ahmad at oracle.com>; serviceability-dev at openjdk.java.net
> Cc: Roger Calnan <roger.calnan at oracle.com>
> Subject: Re: [10] RFR for JDK-8169961: Memory leak after debugging session
> 
> On 8/9/17 08:27, Daniel D. Daugherty wrote:
> > On 8/9/17 3:05 AM, Shafi Ahmad wrote:
> >> May I get it reviewed by  serviceability team.
> >
> > I don't count as being on the Serviceability team anymore, but
> 
> Common, Dan, you are still counted! :)
> 
> > I've pinged Serguei Spitsyn who is back from his vacation...
> >
> > More below...
> 
> More below.
> 
> >
> >
> >>
> >> Regards,
> >> Shafi
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: Shafi Ahmad
> >>> Sent: Wednesday, July 26, 2017 8:26 AM
> >>> To: serviceability-dev at openjdk.java.net
> >>> Cc: Roger Calnan <roger.calnan at oracle.com>
> >>> Subject: RE: [10] RFR for JDK-8169961: Memory leak after debugging
> >>> session
> >>>
> >>> May I get it reviewed by someone from serviceability group.
> >>>
> >>> Webrev link:
> http://cr.openjdk.java.net/~shshahma/8169961/webrev.01/
> >>> This review thread:
> >>> http://mail.openjdk.java.net/pipermail/serviceability-
> >>> dev/2017-July/021538.html
> >>>
> >>> Regards,
> >>> Shafi
> >>>
> >>>> -----Original Message-----
> >>>> From: Shafi Ahmad
> >>>> Sent: Monday, July 24, 2017 12:52 PM
> >>>> To: serviceability-dev at openjdk.java.net
> >>>> Subject: RE: [10] RFR for JDK-8169961: Memory leak after debugging
> >>>> session
> >>>>
> >>>> Hi,
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Langer, Christoph [mailto:christoph.langer at sap.com]
> >>>>> Sent: Monday, July 17, 2017 9:01 PM
> >>>>> To: Poonam Parhar <poonam.bajaj at oracle.com>
> >>>>> Cc: 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
> >>>>>
> >>>>> Hi Poonam,
> >>>>>
> >>>>>
> >>>>>> Line 182: Here, eventController.release() is called after the 'vm'
> >>>>>> is
> >>>> disposed.
> >>>>>> And eventController.release() causes the following statement to
> >>>>>> be executed on the eventcontroller thread after the 'vm' is
> disposed:
> >>>>>>
> >>>>>> JDWP.VirtualMachine.ReleaseEvents.process(vm);
> >>>>>>
> >>>>>> Which does not seem to be right. Someone from the Serviceability
> >>>>>> group can confirm the correctness of this change.
> >>>>> I think this is okay, because with the new change shouldListen()
> >>>>> is called right after the thread returns from wait(). And this
> >>>>> will lead to the thread immediately exiting.
> >>>>> JDWP.VirtualMachine.ReleaseEvents.process(vm);
> >>>>> should not be called in this case.
> >
> > I just re-read the webrev and I agree with Christoph that this new check:
> >
> >   L358:                         if (!shouldListen) {
> >   L359:                            return;
> >   L360:                         }
> >
> > will keep the EventController.run() method from trying to use the 'vm'
> > that has been disposed.
> 
> Agreed.
> The only issue is that the 'shouldListen' field has to be volatile now.
> 
> Otherwise, the fix looks good to me.
> 
> Thank you for taking care about this issue!
> 
> Thanks,
> Serguei
> 
> >
> > Dan
> >
> >
> >>>>>
> >>>>>> Line 330: Instance variable 'VirtualMachineImpl vm' is removed
> >>>>>> from the EventController class. It is being used further down in
> >>>>>> its
> >>>>>> run() method. So I think it cannot be removed.
> >>>>> The vm object is used from the outer class TargetVM, as
> >>>>> EventController is an inner class of it.
> >>>>>
> >>>>> So in my view it's all correct but still somebody of the
> >>>>> serviceability group might know better...
> >>>> Could someone from serviceability group review this.
> >>>>
> >>>> Regards,
> >>>> Shafi
> >>>>
> >>>>> Best regards
> >>>>> Christoph
> >>>>>
> >
>