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. > 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/ 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
