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.daughe...@oracle.com>; Shafi Ahmad > <shafi.s.ah...@oracle.com>; serviceability-dev@openjdk.java.net > Cc: Roger Calnan <roger.cal...@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@openjdk.java.net > >>> Cc: Roger Calnan <roger.cal...@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@openjdk.java.net > >>>> Subject: RE: [10] RFR for JDK-8169961: Memory leak after debugging > >>>> session > >>>> > >>>> Hi, > >>>> > >>>>> -----Original Message----- > >>>>> From: Langer, Christoph [mailto:christoph.lan...@sap.com] > >>>>> Sent: Monday, July 17, 2017 9:01 PM > >>>>> To: Poonam Parhar <poonam.ba...@oracle.com> > >>>>> Cc: Shafi Ahmad <shafi.s.ah...@oracle.com>; serviceability- > >>>>> d...@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 > >>>>> > > >
