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

[Harsha Wardhana B]

Hi Serguei,

Many thanks for reviewing the code.

Regards
Harsha

On Tuesday 22 December 2015 01:31 AM, serguei.spitsyn at 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 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/20151222/48d7bf02/attachment-0001.html>