RFR : JDK-6744127 - NullPointerException at com.sun.tools.jdi.EventRequestManagerImpl.request

[serguei.spitsyn at oracle.com]

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 at 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
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20151221/d385ba5d/attachment.html>