[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: [10] RFR for JDK-8169961: Memory leak after debugging session
From:       "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date:       2017-08-10 7:16:02
Message-ID: 35fc1c90-001e-62b3-9300-9baf83bda878 () oracle ! com
[Download RAW message or body]

Hi Shafi,

Looks good.

Thanks,
Serguei


On 8/10/17 00:08, Shafi Ahmad wrote:
> 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@oracle.com>; Shafi Ahmad
>> <shafi.s.ahmad@oracle.com>; serviceability-dev@openjdk.java.net
>> Cc: Roger Calnan <roger.calnan@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.calnan@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.langer@sap.com]
>>>>>>> Sent: Monday, July 17, 2017 9:01 PM
>>>>>>> To: Poonam Parhar <poonam.bajaj@oracle.com>
>>>>>>> Cc: Shafi Ahmad <shafi.s.ahmad@oracle.com>; serviceability-
>>>>>>> dev@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
>>>>>>>

[prev in list] [next in list] [prev in thread] [next in thread]