RFR (XS): 8014890 : Reference queues may return more entries than expected

[David Holmes]

On 1/07/2013 8:05 PM, Thomas Schatzl wrote:
> Hi David,
>
> On Mon, 2013-07-01 at 17:51 +1000, David Holmes wrote:
>> Well you can ignore what I wrote below - sorry. Somehow I got it in my
>> head that multiple enqueue's were intended/supported when of course they
>> are not. :(
>>
>> So the proposed fix is okay - though I'd simplify the comment to just:
>>
>> // Check that since getting the lock this reference hasn't already been
>> // enqueued (and even then removed)
>
> Done, see the new webrev at
> http://cr.openjdk.java.net/~tschatzl/8014890/webrev.01/
>
> I will send this version to Mandy for pushing if nobody objects in the next few days.

Thanks.

>> The synchronization is problematic as I mention below but there is no
>> easy fix due to the lock-ordering problem, and any attempt at such a fix
>> would be much riskier. So this fix is fine - thanks.
>
> Fyi, while waiting for your approval, I tried to clean up this a little
> taking into account the comments from Peter and Aleksey (sorry if I
> forgot somebody) into account.
>
> A webrev for this is at
> http://cr.openjdk.java.net/~tschatzl/8014890/webrev.refactor/

Yes I think this handles it better. Get rid of synchronization of the 
Reference, make the fields volatile and let things race until the 
ReferenceQueue lock is taken, then sort it out.

David
-----


> The changes are:
>   - avoid the double read from the volatile head member in
> ReferenceQueue.reallyPoll()
>   - the fix for 8014890 you just reviewed
>   - make Reference.queue volatile, and remove the synchronization on it
> from Reference.isEnqueued() and ReferenceQueue.enqueue(). I do not see a
> performance problem: a volatile read (in Reference.isEnqueued()) should
> be at least as fast as the synchronization on that reference.
>
> I think the change is okay as in functionally correct; I ran it through
> jprt (running all the available test cases, including the one for the
> original 8014890 patch, in the process), passing it.
>
> Maybe it is useful to you for future reference.
>
> Thanks everyone for your suggestions,
>    Thomas
>
> Standard information about the patch (the original one, not the
> suggested cleanup):
>
> JIRA:
> https://jbs.oracle.com/bugs/browse/JDK-8014890
>
> bugs.sun.com
> http://bugs.sun.com/view_bug.do?bug_id=8014890
>
> Testing:
> jck, jprt, manual testing on most platforms
>
>