RFR: 7161229 - PriorityBlockingQueue keeps hard reference to last removed element

[Rémi Forax]

Hi David,
I think the test

if (n > 0) {

should not be in siftDown* but should be done at all callsites,
by example in heapify, you can hoist the test outside of the loop.

In dequeue, it's a matter of style but the 'else' is not needed anymore.

Otherwise the changes looks ok for me.

Rémi

On 06/25/2012 06:26 AM, David Holmes wrote:
> webrev:
>
> http://cr.openjdk.java.net/~dholmes/7161229/webrev/
>
> When removing the last element from the PBQ the "sift down" logic 
> would store it back in as array[0]. The simple fix is for the 
> sift-down to be a no-op if the queue size is zero.
>
> The fix has been contributed by Doug Lea. We are taking this 
> opportunity to synchronize PBQ with Doug's latest version at:
>
> http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/java/util/concurrent/PriorityBlockingQueue.java?view=log 
>
>
> So in addition to the above functional fix there are a number of 
> miscellaneous code and comment cleanups. The only non-trivial, but 
> still very simple, change is to the drainTo method to make it more 
> robust if the add() on the destination collection throws an exception.
>
> I am of course a Reviewer for this.
>
> I have provided the update to the LastElement test (as this is 
> certainly related to the last element) but there are some reservations 
> about examining the PBS internals this way. Unfortunately there is no 
> good way to test for these kinds of retention issues. You either need 
> a whitebox test like this, or need to rely on non-guaranteed GC and 
> finalization behaviour.
>
> Thanks,
> David Holmes