Hi Kim,
On 29/07/2015 5:57 PM, Kim Barrett wrote:
Please review this fix of a race condition in
j.l.r.Reference/ReferenceQueue. See comments in the bug report for a
description of the race. The race is being fixed by reordering a pair
of volatile assignments.
While this seems logical for the failure at hand it isn't immediately
obvious to me that setting next before setting the ENQUEUED state won't
cause a problem for some other piece of code. Really these things need
to be tightly synchronized - I think the real bug is the unsynchronized
fast-path for the empty-queue case in poll(). While that change was
deliberate in 6666739 this side-effect was not realized and would have
affected the change. I hope Martin and/or Tom see this and chime in.
That aside the commentary is rather verbose, a simple:
// Only set ENQUEUED state after the reference is enqueued
would suffice (and add "volatiles ensure ordering" if you want that to
be clearer).
Thanks,
David
CR:
https://bugs.openjdk.java.net/browse/JDK-8132306
Webrev:
http://cr.openjdk.java.net/~kbarrett/8132306/webrev.00/
Testing:
jprt with default and hotspot testsets
Locally tested race in original code by insertion of sleep in enqueue and
verifying ReferenceEnqueue regression test fails consistently as
described in the bug report.
Locally tested fixed code by insertion of sleeps at various points and
verifying ReferenceEnqueue regression test still passes.
