Simple FIFO Job list in Java

Question

For the first time I have to play with Threads in java.

Basically, my code is a simple FIFO job queue. The main thread regularly puts jobs in the queue wich need to be executed.

The code below is a striped down version of what I am doing :

public class JobPerformer implements Runnable {
    private final ArrayList<Job> jobList = new ArrayList<Job>();
    private final AtomicBoolean threadStop = new AtomicBoolean(false);
    private Thread thread;
    public JobExecuter(){
       thread = new Thread(this)
       thread.start();
    }

    public void perform(Job job, bool async){
        if(async){
            jobList.add(job);
            jobList.notifyAll();
        }
        else{
            job.perform();
        }
    }

    public void perform(Job job){
        this.perform(job, false);
    }

    public void shutdown(){
         threadStop.set(true);
         if(thread.getState() == Thread.State.WAITING){
             thread.interrupt();
         }
    }

    public void run(){
        while(! threadStop.get()){
            if(jobList.isEmpty()){
                try{
                    jobList.wait();
                } catch (InterruptedException ex) {}
            }
            jobList.get(0).perform();
            jobList.remove(0);
        }

        // Finish the jobs still in the queue
        while(!jobList.isEmpty()){
             jobList.get(0).perform();
             jobList.remove(0);
        }
    }
}

I have two questions :

1. I use wait() and notifyAll() to avoid polling. But nowhere do I synchronize. Since it's a FIFO queue, threads are just adding jobs at the end or reading at the start. I don't see any conflict that would make me say "Hey, I have to make sure that during this op. only one thread can be working on this object", is there a possible conflict I am overseeing that would require me to synchronize anyway ?

2. I am using an "Atomic boolean" because of this question : Multithreading correctly done? What was wrong with using a normal boolean ?

Answer 1

Why don't you use the classes provided in java.util.concurrent that can handle these requirements? Or is this written for practice?

There is still problem if you don't synchronize, because the ArrayList.add() method is not thread-safe thus it can happen that two threads trying to add a new job into the queue nearly the same instant (to the same slot in the array), then it is possible that the one adding later will overwrite the job added by the first thread.

Using normal boolean does not guarantee, that if one thread writes the value, and another accesses immediately after that then it will see the new value. You can use volatile with boolean as well, as in mentioned in the answer you linked. See an explanation for volatile

Or refer to Java Concurrency in Practice, pretty good book.

Answer 2

There are several issues in your code, most importantly:

• don't start a thread in your constructor, it is very possible that jobList.isEmpty() in your run method could throw a NPE because your object has not been fully constructed yet
• the alternatives are: provide a start method, or start the thread the first time perform is called
• you must call wait / notifyAll within a synchronized block or you will get an exception at runtime
• ArrayList is not thread safe so you can't add in the main thread (in perform) and get in the worker thread without synchronization. The result could be anything, including a corruption of the data structure.
• using an ArrayList where you keep adding and removing is not very efficient - you could use a LinkedList instead
• even better, use a BlockingQueue which will handle all the low level synchronization and waiting for you
• if you use a simple boolean for threadStop, it is very possible that your work thread will never see the value become true (even if the main thread calls shutdown)
• instead of AtomicBoolean, you could simply mark the boolean as volatile

Bottom line: as it is your class will throw an exception very quickly. Once you have fixed that, it might work for some time and fail inexplicably, or not work at all.