does anyone see any issues with this thread pattern?

Question

Here is a simple thread pattern that I use when writing a class that needs just one thread, and needs to a specific task.

The usual requirements for such a class are that it should be startable, stopable and restartable. Does anyone see any issues with this pattern that I use?

public class MyThread implements Runnable {
    private boolean _exit = false;
    private Thread _thread = null;

    public void start () {
        _exit = false;

        if (_thread == null) {
            _thread = new Thread(this, "MyThread");
            _thread.start();
        }
    }

    public void run () {
        while (!_exit) {
            //do something
        }
    }

    public void stop () {
        _exit = true;

        if (_thread != null) {
            _thread.interrupt();
            _thread = null;
        }
    }
}

I am looking for comments around if I am missing something, or if there is a better way to write this.

Answer 1

1. Make the boolean flag volatile.
2. When you call stop don't interrupt the thread, but just set the _exit flag to true.
3. If you are going to interrupt, then put a try/catch/finally around the while loop and catch the interrupt exception, cleanup the state of the objects you're working with and exit. And be careful not to cause a deadlock!
4. Finally, you could use a CountDownLatch, or something of the sort, in order to signal that the thread finished.

The other thing is contention... you're not showing anything that will be modified by the thread, so depending on what gets modified you might have to synchronize (lock, etc..).

Answer 2

I'd prefer a guarded block (http://java.sun.com/docs/books/tutorial/essential/concurrency/guardmeth.html) on 'this'. You can notify the thread to come out of the loop very quickly, and then check the 'finished' var again. Where you'd normally use Thread.sleep(x), you use this.wait(x) with a synchronized (this) block around the entire loop. You also need to be in a synchronized (this) block to call this.notifyAll().

Answer 3

The _exit variable should be volatile. Also, it would be useful practice to follow a more normal coding convention. :-)

Answer 4

I would advise against use of the Thread class directly. The Executors framework available since Java 5 simplifies a lot of the issues involved in threading. The idea is that your class would perform the task required, and all of the threading functionality is managed by an Executor, saving you the effort of dealing with the complexity of threading.

A good intro can on the Java Executors framework can be found here.

Answer 5

Well, the class itself is not thread safe. That's not necessarily a problem as long as that's documented and observed in code. If it's not you could lose references to Thread objects that will run in parallel, if two consumers get inside the start() method at the same time.

Flags used as semaphores should also of course be made volatile.

The API of the class is a little bit strange. You implement Runnable, saying to other classes "use my run method to invoke me" but then mimic the start method of a full Thread object. You may want to hide the run method inside an inner class. Otherwise it's somewhat confusing how one is intended to use the object.

And as always, any pattern that involves the words new Thread() rather than using a pool is somewhat suspect in the abstract. Would need to know about what you're actually doing with it to really comment intelligently on that though.

Answer 6

1) You should declare _exit as volatile to prevent thread visibility issues. If stop() may be called by multiple threads, _thread should also be volatile.

2) The call to interrupt will throw an InterruptedException only for interruptible blocking operations. You may need to take more actions, depending on what blocking operations you perform in the thread

3) If you want the class instances to be re-usable, you should set _exit to false in the start() method.