addAll() in LinkedBlockingQueue is thread safe (and solution if it's not)?

Question

Quoting the documentation:

"BlockingQueue implementations are thread-safe. All queuing methods achieve their effects atomically using internal locks or other forms of concurrency control. However, the bulk Collection operations addAll, containsAll, retainAll and removeAll are not necessarily performed atomically unless specified otherwise in an implementation. So it is possible, for example, for addAll(c) to fail (throwing an exception) after adding only some of the elements in c."

Since there isn't written anything in particular in the description of the LinkedBlockingQueue.addAll() operation, I have to assume that's not thread safe.

Do you agree with me that the only solution in order to guarantee that all the elements added through an addAll() are contiguous (i.e. added together) is to use a Lock everytime that the queue is modified (with an add or take operation)? For example:

BlockingQueue<T> queue = new LinkedBlockingQueue<>();
Lock lock = new ReentrantLock();

//somewhere, some thread...
lock.lock();
queue.addAll(someCollection);
lock.unlock();

//somewhere else, (maybe) some other thread...
lock.lock();
queue.take();
lock.unlock();

IMPORTANTE UPDATE: Wow nobody saw a big bug in the previous example: since the take() is a blocking operation, and since the lock is needed in order to add element to the queue, as soon as the queue will be empty the program will enter in a deadlock state: a writer cannot write since the lock is possessed by the take(), and at the same time the take() will be in a blocked state until something is written in the queue (which it can't happen for the previous reason). Any idea? I think that the most obvious one is remove the lock around the take() but then maybe the desired atomicity of addAll() is not guaranteed.

Answer 1

addAll is still thread safe it just does not provide atomicity. So it depends what is your use case / expectation.

If you use addAll without the explicit locking, then if other Thread tries to write to the queue (add new element(s)), the order of added elements is not guaranteed and they may get mixed. If it is an issue than yes you need locking. But addAll is still thread safe, there will be no corruption of the queue.

But typically, the queue is used to provide a way of communication between many Readers/Writers and strict preservation of insertion order may not be needed.

Now, the main problem is, that add method throws an exception if the queue is full, so that addAll operation can collapse in the middle and you don't know which elements were added which weren't.

If your use case allows waiting for the space to insert the elements, than you should rather use put in a loop.

for (E e: someCollection) queue.put(e);

This will block till there is space to add another elements.

Manually locking is tricky, because you always have to remember to add locking when you access the queue, which is error prone. So if you really need the atomicity, write a wrapper class that implements the BlockingQUeue interface but uses locking before calling the underlying operations.

Answer 2

I believe you're confusing thread-safe with atomic. As I understand it, bulk operations are thread-safe, though not atomic.

I don't think you need to use an external ReentrantLock in order to make your BlockingQueue be thread-safe. Actually, addAll() is implemented by iterating over the given Collection and calling add() on the queue for each element of the collection. As add() is thread-safe, then you wouldn't need to synchronize anything.

When the javadocs say this:

So it is possible, for example, for addAll(c) to fail (throwing an exception) after adding only some of the elements in c.

it means that only some elements of the given collection c might have been added when addAll(c) returns. However it doesn't mean that you'd need to lock on the queue to invoke take() or any other operation.

EDIT:

As per your use case, you could use a lock as you suggested, though I would make it internal to the BlockingQueue implementation, so that caller classes wouldn't need to adhere to the lock/call_some_method_from_the_queue/unlock pattern. I'd be more careful when using the lock, i.e. use it in a try/finally block:

public class MyQueue<T> extends LinkedBlockingQueue<T> {

    private final Lock lock = new ReentrantLock();

    @Override
    public boolean addAll(Collection<T> c) {
        boolean r = false;
        try {
            this.lock.lock();
            r = super.addAll(c);
        } finally {
            this.lock.unlock();
        }
        return r;
    }

    @Override
    public void add(T e) {
        try {
            this.lock.lock();
            super.add(e);
        } finally {
            this.lock.unlock();
        }
    }

    // You don't need to lock on take(), since
    // it preserves the order in which elements
    // are inserted and is already thread-safe
}