[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR : JDK-6744127 - NullPointerException at com.sun.tools.jdi.EventRequestManagerImpl.request
From: Harsha Wardhana B <harsha.wardhana.b () oracle ! com>
Date: 2015-12-22 4:18:13
Message-ID: 5678CC35.601 () oracle ! com
[Download RAW message or body]
Hi Serguei,
Many thanks for reviewing the code.
Regards
Harsha
On Tuesday 22 December 2015 01:31 AM, serguei.spitsyn@oracle.com wrote:
> Hi Harsha,
>
>
> The fix is good, thumbs up.
> Thank you for the extra work around the test failure!
>
> Thanks,
> Serguei
>
>
>
> On 12/21/15 05:37, Harsha Wardhana B wrote:
>> Hi All,
>>
>> Please find below the updated webrev incorporating review comments.
>>
>> Issue : https://bugs.openjdk.java.net/browse/JDK-6744127
>> webrev :
>> http://cr.openjdk.java.net/~jbachorik/sponsorship/6744127/webrev.01/
>>
>> Hello Serguei,
>>
>> I will send fix for tonga test case in a separate review request.
>>
>> Regards
>> Harsha
>>
>> On Tuesday 15 December 2015 01:31 PM, serguei.spitsyn@oracle.com wrote:
>>> Hi Harsha,
>>>
>>> The fix looks good in general.
>>>
>>> If I understand correctly, any invoke of the requestList().add(),
>>> requestList().remove()
>>> or requestList().clear() will be implicitly synchronized on the list
>>> object.
>>> There only place left where we still need an explicit synchronized
>>> statement
>>> is the iteration over the list in the body of the method:
>>> EventRequest request(int eventCmd, int requestId);
>>>
>>> I have some minor comments, though.
>>>
>>> The comment at the line 936 needs a dot at the end.
>>> A space is needed after the keywords "synchronized", "while" and
>>> "if" (lines: 943, 945 and 947).
>>> The space is not needed at the line 946 after the cast.
>>>
>>> Also, I'd use the for-loop instead of while-loop like this:
>>> for (EventRequestImpl er: rl) {
>>> if (er.id == requestId) {
>>> return er;
>>> }
>>> }
>>>
>>> But I leave it up to to you if you like to keep your variant.
>>>
>>> What tests did you run to make sure the fix does not brake anything?
>>> I'd recommend to run the JTreg com/sun/jdi tests and the
>>> co-located nsk.jdi.testlist tests.
>>> If you already done so, could you, please, share the results?
>>>
>>>
>>> Thank you for fixing this bug!
>>> Serguei
>>>
>>>
>>>
>>> On 12/13/15 23:32, Harsha Wardhana B wrote:
>>>> Hi All,
>>>>
>>>> Please review the fix for bug,
>>>>
>>>> Issue : https://bugs.openjdk.java.net/browse/JDK-6744127
>>>> Webrev :
>>>> http://cr.openjdk.java.net/~jbachorik/sponsorship/6744127/webrev.00
>>>>
>>>> The fix synchronizes access to requestList of EventRequestManagerImpl.
>>>>
>>>> Thanks
>>>> Harsha
>>>>
>>>
>>
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hi Serguei,<br>
<br>
Many thanks for reviewing the code.<br>
<br>
Regards<br>
Harsha<br>
<br>
<div class="moz-cite-prefix">On Tuesday 22 December 2015 01:31 AM,
<a class="moz-txt-link-abbreviated" \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote:<br> \
</div> <blockquote cite="mid:56785A94.2010702@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<div class="moz-cite-prefix">Hi Harsha,<br>
<br>
<br>
The fix is good, thumbs up.<br>
Thank you for the extra work around the test failure!<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
<br>
On 12/21/15 05:37, Harsha Wardhana B wrote:<br>
</div>
<blockquote cite="mid:567800B4.9010202@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
Hi All,<br>
<br>
Please find below the updated webrev incorporating review
comments. <br>
<br>
Issue : <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-6744127">https://bugs.openjdk.java.net/browse/JDK-6744127</a><br>
webrev : <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejbachorik/sponsorship/6744127/webrev.01/">http://cr.openjdk.java.net/~jbachorik/sponsorship/6744127/webrev.01/</a><br>
<br>
Hello Serguei,<br>
<br>
I will send fix for tonga test case in a separate review
request.<br>
<br>
Regards<br>
Harsha<br>
<br>
<div class="moz-cite-prefix">On Tuesday 15 December 2015 01:31
PM, <a moz-do-not-send="true"
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
wrote:<br>
</div>
<blockquote cite="mid:566FC8CC.8030109@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">Hi Harsha,<br>
<br>
The fix looks good in general.<br>
<br>
If I understand correctly, any invoke of the
requestList().add(), requestList().remove()<br>
or requestList().clear() will be implicitly synchronized on
the list object.<br>
There only place left where we still need an explicit
synchronized statement<br>
is the iteration over the list in the body of the method:<br>
EventRequest request(int eventCmd, int requestId);
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
<br>
<br>
I have some minor comments, though.<br>
<br>
The comment at the line 936 needs a dot at the end.<br>
A space is needed after the keywords "synchronized",
"while" and "if" (lines: 943, 945 and 947).<br>
The space is not needed at the line 946 after the cast.<br>
<br>
Also, I'd use the for-loop instead of while-loop like
this:<br>
for (<span class="changed">EventRequestImpl er: rl) {<br>
if (er.id == requestId) {<br>
return er;<br>
}<br>
}<br>
<br>
But I leave it up to to you if you like to keep your
variant.<br>
<br>
What tests did you run to make sure the fix does not
brake anything?<br>
I'd recommend to run the JTreg com/sun/jdi tests and
the co-located nsk.jdi.testlist tests.<br>
If you already done so, could you, please, share the
results?<br>
<br>
<br>
Thank you for fixing this bug!<br>
Serguei</span>
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
<br>
<br>
<br>
<br>
On 12/13/15 23:32, Harsha Wardhana B wrote:<br>
</div>
<blockquote cite="mid:566E70A1.10701@oracle.com" type="cite">Hi
All, <br>
<br>
Please review the fix for bug, <br>
<br>
Issue : <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-6744127">https://bugs.openjdk.java.net/browse/JDK-6744127</a>
<br>
Webrev : <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejbachorik/sponsorship/6744127/webrev.00">http://cr.openjdk.java.net/~jbachorik/sponsorship/6744127/webrev.00</a>
<br>
<br>
The fix synchronizes access to requestList of
EventRequestManagerImpl. <br>
<br>
Thanks <br>
Harsha <br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic