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.