CODEWITHSUNDEEP
javamultithreadingthread-safety
Mark Estrada
Mark Estrada

Reputation: 9191

Alternative to calling a thread in class constructor

I have a class that is being accessed by multiple threads.. I wanted the class to do something before it can respond to call (getSomething).

I was thinking of starting my SampleThreadinside the class constructor but I dont like the idea of firing up a thread inside a constructor.

I am thinking of doing something like this but I am not sure if this is correct. The first thread that will call getSomething on my class will start a thread..

But I am still unsure if this is correct..I worry that multiple SampleThread will run and I wanted it to run only once.

public class A{
    private final AtomicBoolean isReady = new AtomicBoolean(false);
    public A{

    }

    public void getSomething(){
        if(!isReady.get()){
            new SampleThread().start();
        }
        //continue with the rest of the method
    }
}

public class SampleThread{
    public void run(){
        //Do some long running task once done
        isReady.set(true);
    }
}

I dont have a way to add a method called start() where I could call my SampleThread since this is being called by the framework.

Any hints?

UPDATE 2

I tried a sample class to simulate this, and I used a latch to wait for my InitializerThread to finish.

package com.race;

import java.util.concurrent.CountDownLatch;
import java.util.concurrent.atomic.AtomicBoolean;

public class TestRaceCondition {

    public static class SampleClass implements Runnable {
        private final CountDownLatch latch = new CountDownLatch(1);
        private final AtomicBoolean isReady = new AtomicBoolean(false);

        public void doSomething() {

            synchronized (this) {
                if (!isReady.get()) {
                    System.out.println(Thread.currentThread().getName() + " is initializing the system....");
                    new InitializerThread(latch).start();

                    try {
                        latch.await();
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                    isReady.set(true);
                }
            }

            System.out.println("Doing something...." + Thread.currentThread().getName());
            System.out.println("Still doing something...." + Thread.currentThread().getName());

        }

        @Override
        public void run() {
//          System.out.println(Thread.currentThread().getName() + " :: is running!");
            doSomething();
        }

    }

    public static class InitializerThread extends Thread {
        private CountDownLatch latch;

        public InitializerThread(CountDownLatch latch) {
            this.latch = latch;
        }

        @Override
        public void run() {
            // Simulate some long running task
            System.out.println(Thread.currentThread().getName() + " is calling a long running task....");
            try {
                Thread.sleep(2000);
            } catch (InterruptedException e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
            }
            latch.countDown();
        }

    }

    public static void main(String[] args) {
        SampleClass myClass = new SampleClass();
        Thread t1 = new Thread(myClass);
        Thread t2 = new Thread(myClass);
        Thread t3 = new Thread(myClass);
        Thread t4 = new Thread(myClass);

        t1.start();
        t2.start();
        t3.start();
        t4.start();
    }
}

But I am not sure whats wrong with the result..

Initializing system....
Calling a long running task....
Initializing system....
Doing something....Thread-0
Still doing something....Thread-0
Doing something....Thread-3
Still doing something....Thread-3
Calling a long running task....
Initializing system....
Doing something....Thread-2
Still doing something....Thread-2
Calling a long running task....
Initializing system....
Doing something....Thread-1
Still doing something....Thread-1
Calling a long running task....

It seems that it is still calling my intializer thread multiple times... I added the latch but something is wrong with my output.

I was expecting something like this...

Initializing system....
Calling a long running task....
Doing something....Thread-0
Still doing something....Thread-0
Doing something....Thread-3
Still doing something....Thread-3
Doing something....Thread-2
Still doing something....Thread-2
Doing something....Thread-1
Still doing something....Thread-1

UPDATE 3 I edited the code as per recommendation to include the synchronize...

Thread-0 is initializing the system....
Thread-4 is calling a long running task....
Doing something....Thread-0
Still doing something....Thread-0
Doing something....Thread-2
Still doing something....Thread-2
Doing something....Thread-3
Still doing something....Thread-3
Doing something....Thread-1
Still doing something....Thread-1

I just don't get the output..why Thread-0 is doing the initialization but Thread-4 is running the task. I was expecting the first thread to do the initialization and calling of the long running task.

Upvotes: 0

Views: 63

Answers (1)

davidxxx
davidxxx

Reputation: 131346

This way has a race condition :

public void getSomething(){
    if(!isReady.get()){
        new SampleThread().start();
    }
    //continue with the rest of the method
}

This is atomic : if(!isReady.get()) but the body of the conditional statement associated to is not : 

{
    new SampleThread().start();
}

So you could start twice this thread.

Synchronizing the logic prevents the race condition. It will also increase the number of potential locks on the object but as if(!isReady.get()) should be fast executed, it should be acceptable.
Note that you probably don't need to use an AtomicBoolean if the boolean value is used only in synchronized statements.

So here two ways according to your requirements.

1) To allow the first invocation of getSomething() to start SampleThread and that others threads wait for the end of this initialization before executing getSomething() :

public void getSomething(){
    synchronized(this){
      // init the logic 
      if(!isReady){
          SampleThread t = new SampleThread();
          t.start(); 
          t.join();  // wait for the SampleThread thread termination
          isReady.set(true);           
      }         
      // execute the next only as the init thread was terminated
      if(isReady){
         //continue with the rest of the method
      }

    }     
}

2) To allow the first invocation of getSomething() to start SampleThread and that others threads don't wait for the end of this initialization before executing getSomething() :

public void getSomething(){
    synchronized(this){
      // init the logic once
      if(!isReady.get()){
          SampleThread t = new SampleThread();
          t.start();                                   
      }                                   
    }
    //continue with the rest of the method       
}

And set isReady to true at the end of run() of SampleThread :

public void run(){
    //Do some long running task once done
    isReady.set(true);
}

Upvotes: 2

Related Questions