How can thread already be started?

Question

I have a single instance thread class.

public class LogThread extends Thread{

    private static LogThread instance = null;
    private volatile boolean isRunning = false;
    private final static Object instanceLock = new Object();

    public static synchronized LogThread getInstance(){
        synchronized(instanceLock){
            if(instance == null)
                instance = new LogThread();
        }
        return instance;
    }

    @Override
    public run(){
        //Doing some run stuff
        //Once run is finished

        synchronized(instanceLock){
            isRunning = false;
            instance = null;
        }
    }

    @Override
    public synchronized void start() {
        synchronized (instanceLock){
            if(!isRunning){
                isRunning = true;
                super.start();
            }
        }
    }
}

I am calling start from another thread each time i get the instance, and every once in a while i get a IllegalThreadStateException in com.......LogThread.start, line x Thread already started.

How can the thread already be started if I set isRunning before starting the thread AND synchronize it based on instanceLock.

EDIT:: I have edited my getInstance() to below:

 public static synchronized LogThread getInstance(){
     synchronized(instanceLock){
         if(instance == null){
             instance = new LogThread();
             instance.start();
         }
     }
 }

it should stop any attempts to start a thread that is already started.

Answer 1

The sequence would be this:

• Thread A calls getInstance. Thread A takes the lock for a while, time to check that instance is not null, which it isn't at this point.
• Thread LogThread completes it's execution, isRunning is set to false. instance is set to false, but is still retained by Thread A
• Thread A calls start on the instance. isRunning is false, therefore start is called, hence the crash.

A quick fix would be to not set isRunning to false, since there is never any good reason for that instance to be available for starting again. It should then be renamed isStarted for consistency with what it does.

A proper solution would be to use a newSingleThreadExecutor, as suggested by @Mani

Answer 2

It is never legal to start a thread more than once.
     * In particular, a thread may not be restarted once it has completed
     * execution.

From Thread.start

You should not start the thread , which is already started. even though the thread completes it work ( run completed).

It simply checks the threadStatus and throw exception

  if (threadStatus != 0)
            throw new IllegalThreadStateException();

If i understands clearly, you want to make sure only one thread spend for this task, and you want to reuse the same thread. IMHO , you have two choices,

Option 1: Create new thread each time. make sure the previous thread is completed its work. It is expensive since creating new thread each time is expensive

Option 2: Executors.newSingleThreadExecutor() use this. this will make sure only one thread is created.

And in general , Avoid extend thread, use Runnable .

If you want to use only one thread. and have lot of Jobs. you could use the following.

class LogJob implements Runnable{
    @Override
    public void run() {
        // Do your Job
    }
}

  ExecutorService singleThread = Executors.newSingleThreadExecutor();
LogJob job = new LogJob();
for (int i=0;i<100;i++){
    singleThread.submit(job);
}
singleThread.shutdown();

// For Demo i have put it in loop. You can just call singleThread.submit(job); . At max only one Job would run at any given time. and all your jobs would compelte.

You don't need any Sync block / method