ArrayBlockingQueue NoSuchElementException

Question

Just for learning, I have written the following code for custom thread pool referring and editing the code shown here.

As shown in the code I am using ArrayBlockingQueue for task queue.

Code:

import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.TimeUnit;

public class ThreadPoolService {
    private final BlockingQueue<Runnable> taskQueue;
    private final int corePoolSize;

    private ThreadPoolService(int corePoolSize) {
        this.corePoolSize = corePoolSize;
        this.taskQueue = new ArrayBlockingQueue<>(corePoolSize);
        ThreadPool[] threadPool = new ThreadPool[corePoolSize];
        for (int i = 0; i < corePoolSize; i++) {
            threadPool[i] = new ThreadPool();
            threadPool[i].start();
        }
    }

    public static ThreadPoolService newFixedThreadPool(int size) {
        return new ThreadPoolService(size);
    }

    public void execute(Runnable task) {
        try {
            taskQueue.offer(task, 10, TimeUnit.SECONDS);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }

    private class ThreadPool extends Thread {
        Runnable task;

        @Override
        public void run() {
            while (true) {
                try {
                    while (!taskQueue.isEmpty()) {
                        task = taskQueue.remove();
                        task.run();
                    }
                } catch (RuntimeException ex) {
                    ex.printStackTrace();
                }
            }
        }
    }

    public static void main(String[] args) {
        ThreadPoolService pool = ThreadPoolService.newFixedThreadPool(10);
        Runnable task1 = () -> {
            System.out.println(" Wait for sometime: -> " + Thread.currentThread().getName());
            try {
                TimeUnit.SECONDS.sleep(2);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        };

        Runnable task2 = () -> System.out.println(" Do  Task 2 -> " + Thread.currentThread().getName());
        Runnable task3 = () -> System.out.println(" Do  Task 3 -> " + Thread.currentThread().getName());
        Runnable task4 = () -> System.out.println(" Do  Task 4 -> " + Thread.currentThread().getName());
        List<Runnable> taskList = new ArrayList<>();
        taskList.add(task1);
        taskList.add(task2);
        taskList.add(task3);
        taskList.add(task4);
        for (Runnable task : taskList) {
            pool.execute(task);
        }
    }
}

This code runs fine sometimes and sometimes gives an error.

Success output:

Do  Task 2 -> Thread-2
Wait for sometime: -> Thread-8
Do  Task 3 -> Thread-6
Do  Task 4 -> Thread-7

Failure output:

Do  Task 4 -> Thread-3
Do  Task 3 -> Thread-6
Wait for sometime: -> Thread-4
Do  Task 2 -> Thread-7
java.util.NoSuchElementException
    at java.util.AbstractQueue.remove(AbstractQueue.java:117)
    at com.interview.java.ThreadPoolService$ThreadPool.run(ThreadPoolService.java:43)
java.util.NoSuchElementException
    at java.util.AbstractQueue.remove(AbstractQueue.java:117)
    at com.interview.java.ThreadPoolService$ThreadPool.run(ThreadPoolService.java:43)
java.util.NoSuchElementException
    at java.util.AbstractQueue.remove(AbstractQueue.java:117)
    at com.interview.java.ThreadPoolService$ThreadPool.run(ThreadPoolService.java:43)

I see the reason for the error is the attempt to remove the element when the queue is empty. But it should not because i am doing queue empty check at line no 42 (while (!taskQueue.isEmpty())). What is wrong with code and also why it runs without error sometimes ?

Answer 1

Between the 'while' check and the actual removal, the queue could be modified by another Thread, possibly resulting in the error you mention. That's called a 'race condition'.

So in order to fix that you'll need a way to block the access to the queue by other threads, either by 'locking', using a 'synchronized' block with a shared lock object. Or simply by 'polling' instead of removing.

Answer 2

BlockingQueue is thread-safe only on an individual operation level.I'm seeing check-then-actoperation in the code which is a compound operation which is not thread-safe. To make this code thread-safe perform the check-then-act inside a synchronized block and lock on the queue itself.

synchronized(taskQueue) {
       while (!taskQueue.isEmpty()) {
             task = taskQueue.remove();
             task.run();
 }};

optimization: If the task is a time consuming one, you can execute it outside the synchronized block. So that other threads don't have to wait till the current task completes.

Answer 3

What is wrong with code?

You access taskQueue field from several threads without synchronization. You must do queue empty check and remove operation atomically, which can be done with synchronized keyword:

private class ThreadPool extends Thread {

    @Override
    public void run() {
        Runnable task;

        while (true) {
            synchronized(queue) {
                // give access to taskQueue to one thread at a time 
                if (!taskQueue.isEmpty()) {
                    task = taskQueue.remove();
                }
            }

            try {
                task.run();
            } catch (RuntimeException ex) {
                ex.printStackTrace();
            }
        }
    }
}

why it runs without error sometimes?

Because of nature of JVM thread scheduler: sometimes it plans thread execution in such a way that they access taskQueue synchronously by themselves. But when you deal with multithreading, you can't rely on thread execution order, and must synchronize access to shared objects by yourself.