On Tue, 10 Jun 2025 16:03:47 GMT, kabutz <d...@openjdk.org> wrote:
>> We logged several bugs on the LinkedBlockingDeque. This aggregates them into
>> a single bug report and PR.
>>
>> 1. LinkedBlockingDeque does not immediately throw InterruptedException in
>> put/take
>>
>> The LinkedBlockingDeque does not behave consistently with other concurrency
>> components. If we call putFirst(), putLast(), takeFirst(), or takeLast()
>> with a thread that is interrupted, it does not immediately throw an
>> InterruptedException, the way that ArrayBlockingQueue and
>> LInkedBlockingQueue does, because instead of lockInterruptibly(), we call
>> lock(). It will only throw an InterruptedException if the queue is full (on
>> put) or empty (on take). Since interruptions are frequently used as a
>> shutdown mechanism, this might prevent code from ever shutting down.
>>
>> 2. LinkedBlockingDeque.clear() should preserve weakly-consistent iterators
>>
>> LinkedBlockingDeque.clear() should preserve weakly-consistent iterators by
>> linking f.prev and f.next back to f, allowing the iterators to continue from
>> the first or last respectively. This would be consistent with how the other
>> node-based weakly consistent queues LinkedBlockingQueue LinkedTransferQueue,
>> ConcurrentLinkedQueue/Deque work.
>>
>> The LBD already supports self-linking, since that is done by the
>> unlinkFirst() and unlinkLast() methods, and the iterators and spliterator
>> thus all support self-linking.
>>
>> This can be fixed very easily by linking both f.prev and f.next back to f.
>>
>> 3. LinkedBlockingDeque offer() creates nodes even if capacity has been
>> reached
>>
>> In the JavaDoc of LinkedBlockingDeque, it states: "Linked nodes are
>> dynamically created upon each insertion unless this would bring the deque
>> above capacity." However, in the current implementation, nodes are always
>> created, even if the deque is full. This is because count is non-volatile,
>> and we only check inside the linkFirst/Last() methods whether the queue is
>> full. At this point we have already locked and have created the Node.
>> Instead, the count could be volatile, and we could check before locking.
>>
>> In the current version, calling offer() on a full LinkedBlockingDeque
>> creates unnecessary objects and contention. Similarly for poll() and peek(),
>> we could exit prior to locking by checking the count field.
>>
>> 4. LinkedBlockingDeque allows us to overflow size with addAll()
>>
>> In LinkedBlockingDeque.addAll() we first build up the chain of nodes and
>> then add that chain in bulk to the existing nodes. We count the nodes in
>> "int n" and then whilst hol...
>
> kabutz has updated the pull request incrementally with two additional commits
> since the last revision:
>
> - Removed sizes from LBD constructors - not necessary
> - More optimizing volatile reads
test/jdk/java/util/concurrent/tck/LinkedBlockingDequeTest.java line 1904:
> 1902: fail("Expected InterruptedException in putFirst()");
> 1903: } catch (InterruptedException expected) {
> 1904: // good that's what we want
@kabutz I think it makes sense to verify that throwing the exception also
clears the interrupt.
Suggestion:
// good that's what we want
assertFalse(Thread.currentThread().isInterrupted());
test/jdk/java/util/concurrent/tck/LinkedBlockingDequeTest.java line 1912:
> 1910: fail("Expected InterruptedException in putLast()");
> 1911: } catch (InterruptedException expected) {
> 1912: // good that's what we want
Suggestion:
// good that's what we want
assertFalse(Thread.currentThread().isInterrupted());
test/jdk/java/util/concurrent/tck/LinkedBlockingDequeTest.java line 1921:
> 1919: fail("Expected InterruptedException in
> takeFirst()");
> 1920: } catch (InterruptedException expected) {
> 1921: // good that's what we want
Suggestion:
// good that's what we want
assertFalse(Thread.currentThread().isInterrupted());
test/jdk/java/util/concurrent/tck/LinkedBlockingDequeTest.java line 1930:
> 1928: fail("Expected InterruptedException in takeLast()");
> 1929: } catch (InterruptedException expected) {
> 1930: // good that's what we want
Suggestion:
// good that's what we want
assertFalse(Thread.currentThread().isInterrupted());
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24925#discussion_r2139634311
PR Review Comment: https://git.openjdk.org/jdk/pull/24925#discussion_r2139636727
PR Review Comment: https://git.openjdk.org/jdk/pull/24925#discussion_r2139637138
PR Review Comment: https://git.openjdk.org/jdk/pull/24925#discussion_r2139637525
