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
