Runnable/Thread possible misconception

Question

Not sure I understand threads correctly, could someone tell me whether I´m right or wrong in the following example:

class Task {
String taskName;
private Thread thread;
boolean isFinished;

public Task(String name){
    taskName = name;
}

public void createTask(final Runnable r) {
    thread = new Thread(r){
        public void run(){
            if(r != null) {
                r.run();
                isFinished = true;
            }
        }
    };
    thread.start();
}
}

What I'm actually doing in my app is that I set the isFinished to true, and have an observer that whenever the isFinished is true does some stuff. I'm afraid that isFinished is set to true, before all code in the Runnable I pass as parameter is actually terminated.

Isn't the run method suppose to put the code I pass, in a separate thread and run that code asynchronously?

Answer 1

Close, but your new thread is already given the runnable object to execute. You really want to give it a wrapper which runs the r.run() method and then sets isFinished.

Change:

public void createTask(final Runnable r) {
    thread = new Thread(r){
        public void run(){
            if(r != null) {
                r.run();
                isFinished = true;
            }
        }
    };
    thread.start();
}

to

public void createTask(final Runnable r) {
    thread = new Thread( new Runnable {
        public void run(){
            if(r != null) {
                r.run();
                isFinished = true;
            }
        }
    });
    thread.start();
}

I would be remiss if I didn't point out the thread-unsafetiness of isFinished. You will not be guaranteed to notice when the thread finishes, without adding synchronization. I recommend you add:

public synchronized boolean getIsFinished()
{
    return isFinished;
}

public synchronized void setIsFinished(boolean finished)
{
    isFinished = finished;
}

And use these methods to get or set the isFinished flag. Given your lack of synchronization here, you may be seeing other thread-safety oddities, depending on whether or not your r.run() method and your other "observer" are sharing data without synchronization as well.

Answer 2

You should almost never pass a Runnable into the constructor of a Thread and override the Thread's run() method.

The following two pieces of code are essentially identical:

Runnable r = new Runnable( )
{
    public void run( )
    {
        // do stuff...
    }
};

new Thread( r ).start( );

An here's another way to accomplish the same thing by overriding run():

(new Thread( )
{
    public void run( )
    {
        // do stuff...
    }
}).start( );

Answer 3

No, the run method simply is a normal function, that you can override when extending the Thread class in order to implement your own behaviour.

It's the start method of the Thread class that starts a new thread and runs that code async.

Answer 4

Well your code is partially right, and partially wrong.

You are correct that isFinished will only be set to true once everything inside the runnable you are passing in the parameter has finished executing.

However, due to the particular semantics of the java memory model (I'll get into more details about that below), it is possible that when you set isFinished to true, that change is only visible to the thread that has set that variable to true. If you want your code to work as expected, you need to declare isFinished as volatile. This will make any changes you make to that variable immediately visible by other threads.

Another way to do it is to declare isFinished as AtomicBoolean rather than boolean. This class has many methods that allow you to check and set the boolean in an atomic way, helping you to avoid many common multithreading pitfalls.

Answer 5

The way you wrote your code, isFinished will not be set to true until r.run() is complete. It may be appearing otherwise because you may have some data visibility issues due to missing synchronization or missing volatile declarations.

It's a little bit odd since you're both passing in the Runnable to the constructor, but calling it using the reference from your method declaration, not the one inside the thread. But it "works", there's just a redundancy there.

As an aside, don't forget @Override in your anonymous class :)

Answer 6

I suggest you use the synchronization primitive specifically designed for your problem.

This primitive is called CountDownLatch.

Here is the updated code:

class Task {

  String taskName;
  private Thread thread;
  CountDownLatch finishedSignal = new CountDownLatch( 1 );

  public Task(String name){
    taskName = name;
  }

  public void createTask(final Runnable r) {
    thread = new Thread(r){
        public void run(){
            if(r != null) {
                r.run();
                finishedSignal.countDown( );
            }
        }
    };
    thread.start();

    finishedSignal.await( );
  }
}

Answer 7

You should use a FutureTask instead of your own Task class. It has an isDone() method, and it is integrated nicely with the Executor framework.

Most importantly the happens-before relationships are maintained as you expect it (actually in your code the problem is not that isFinished is set to true, before all code in the Runnable is terminated, but the other way: possibly it will not be set to true in the original thread even if the Runnable is terminated)

Example:

Runnable runnable = new Runnable() {
    @Override
    public void run() {
        try {
            Thread.sleep(3000);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
        System.out.println("work done");
    }
};

FutureTask<Void> task = new FutureTask<Void>(runnable, null);
ExecutorService es = Executors.newSingleThreadExecutor();
es.submit (task);

while (!task.isDone()) {
    System.out.println("waiting...");
    try {
        Thread.sleep(500);
    } catch (InterruptedException e) {
        e.printStackTrace();
    }
}