Thread Safe Service to Perform Tasks in a Queue

Question

In answering a question on SO, I provided this example. The intention was to provide a thread safe service that queues requests to make directories. I figured I could definitely use some review on my use of threading, singletons and general Java skills.

package mkdir;

import java.io.File;
import java.util.concurrent.ConcurrentLinkedQueue;

public class MkDirService implements Runnable {

    private static MkDirService service;
    private static ConcurrentLinkedQueue<File> pendingDirs;


    private MkDirService() {
        pendingDirs = new ConcurrentLinkedQueue<File>();
    }

    public static MkDirService getService() {
        if (service == null) {
            service = new MkDirService();
            new Thread(service).start();
        }
        return service;

    }

    public void makeDir(File dir) {
        pendingDirs.add(dir);
    }

    @Override
    public void run() {
        while (true) {
            while (!pendingDirs.isEmpty())
            {
                File curDir = pendingDirs.poll();
                if (curDir !=null && !curDir.exists()) {
                    curDir.mkdir();
                }
            }
            try {
                Thread.sleep(100);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }

    }

}

Answer 1

pendingDirs should not be static. It's one thing to maintain a static instance to implement a singleton, but that object should interact with its state using normal instance variables. And you may as well initialize it in its declaration instead of the constructor.

The service should create the thread itself instead of creating it in getService() to make it self-contained. This allows another implementation to use a thread pool for parallel directory creation and easier testing.

getService() is not thread-safe and could allow two or more instances to be created.

Use a blocking queue, so you don't have to poll and sleep. The thread will be put to sleep if the queue is empty and be awoken once a new directory is added to the queue. This will shrink your run loop to a few lines and allow you to rely on the well-tested thread-management in java.util.concurrent.

Answer 2

Also to create even one thread it is recommended to use concurrent package: Executors.newSingleThreadExecutor().submit(new MkDirService());