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, [email protected] 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
