HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Thread Safe Service to Perform Tasks in a Queue

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
tasksservicethreadsafeperformqueue

Problem

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 pendingDirs;

    private MkDirService() {
        pendingDirs = new ConcurrentLinkedQueue();
    }

    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();
            }
        }

    }

}

Solution

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.

Context

StackExchange Code Review Q#1150, answer score: 2

Revisions (0)

No revisions yet.