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

[serguei.spitsyn at oracle.com]

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/20151215/76dcce2e/attachment.html>