Why Not to Start a Thread in the Constructor? How to Terminate

Why not to start a thread in the constructor? How to terminate?

To your first question: Starting a thread in a constructor passing in this escapes this. That means that you are actually giving out a reference to your object before it is fully constructed. The thread will start before your constructor finishes. This can result in all kinds of weird behaviors.

To your second question: There is no acceptable way to force another thread to stop in Java, so you would use a variable which the thread would check to know whether or not it should stop. The other thread would set it to indicate that the first thread would stop. The variable has to be volatile or all accesses synchronized to ensure proper publication. Here is some code which would be something like what you want.

public class MyNewThread implements Runnable {

    private final Thread t;
    private volatile boolean shouldStop = false;

    MyNewThread() {
        t = new Thread (this, "Data Thread");
    }

    public void start() {
        t.start();
    }

    public void stop() {   
         shouldStop = true;
    }

    public void run()  {
         while(!shouldStop)
         {
             // do stuff
         }
    }
}

Whatever wants to create and start the thread would do:

MyNewThread thread = new MyNewThread();
thread.start();

Whatever wants to stop the thread would do:

thread.stop();

Why shouldn't I use Thread.start() in the constructor of my class?

But the object has been fully constructed, the constructor has nothing left to do but return

Yes and no. The problem is that according to the Java memory model, the compiler is able to reorder the constructor operations and actually finish the constructor of the object after the constructor finishes. volatile or final fields will be guaranteed to be initialized before the constructor finishes but there is no guarantee that (for example) your ImportantData data field will be properly initialized by the time the constructor finishes.

However as @meriton pointed out in comments, there is a happens before relationship with a thread and the thread that started it. In the case of #2, you are fine because data has to be assigned fully before the thread is started. This is guaranteed according to the Java memory model.

That said, it is considered bad practice to "leak" a reference to an object in its constructor to another thread because if any constructor lines were added after the t.start() it would be a race condition if the thread would see the object full constructed or not.

Here's some more reading:

• Here's a good question to read: calling thread.start() within its own constructor
• Doug Lea's memory model page talks about instruction reordering and constructors.
• Here's a great piece about safe constructor practices which talks about this more.
• This is the reason why there are problems with the "double check locking" problem as well.
• My answer to this question is relevant: Is this a safe publication of object?

Why it is bad practice to create a new thread on constructors?

Why it is bad practice to create a new thread on constructors?

Findbugs is alerting you to the problem with instruction reordering possibilities around object construction. Although the memory space for a new object is allocated, there is no guarantee that any of the fields have been initialized by the time your InnerThread has been started. Although the final fields will be initialized before the constructor finishes, there is no guarantee that if InnerThread starts to use (for example) aField when it starts, that it will be initialized. The Java compiler does this for performance reasons. It also has the option to move the initialization of non-final fields to after the new instance has been returned by the constructor.

If you start a new thread in the constructor then there is a chance that the thread will be dealing with a partially initialized object. Even if the thread.start() is the last statement in your constructor, the new thread may be accessing a partially constructed object because of the reordering. This is part of the Java language specification.

Here's a good link about the topic: calling thread.start() within its own constructor

It mentions the following:

By starting it from within the constructor, you are guaranteed to violate the Java Memory Model guidelines. See Brian Goetz's Safe Construction Techniques for more info.

Edit:

Since you code is starting a new thread that is accessing afield, according to the Java Memory Model there is no guarantee that afield will be properly initialized when the thread starts to run.

What I would recommend instead is to add a start() method on your class that calls the thread.start(). This is a better practice and makes it more visible to other classes that are using this class that a thread is created in the constructor.

run() is never called by Thread.start() method

run() is never called by Thread.start() method

You code actually works on my system but that it doesn't work on your's, demonstrates that you have a classic race condition.

Inside of main(), the NewThread is constructed but the Java language says that it can reorder operations so that the operations in a constructor can happen after the constructor finishes. So it is possible that main() might finish before the NewThread has actually been started which can result in the JVM shutting down without running the thread.

Because of instruction reordering, you should never have a thread auto-start itself inside of the constructor. See: Why not to start a thread in the constructor? How to terminate?

You should instead do:

public NewThread() {
    t = new Thread(this, "Thread created by Thread Class.");
    System.out.println("Created by constuctor:" + t);
    // don't start here
}
public void start() {
    // start in another method
    t.start();
}
public void run() {     
    System.out.println("run() method called.");     
}
...

public static void main(String[] args) {
    NewThread nt = new NewThread();
    nt.start();
}

Since the NewThread has the same daemon status as your main thread (which is non-daemon) the JVM will not shutdown until nt.run() completes.

Understanding why is it unsafe to start a thread inside a constructor in terms of the Java memory model

Suppose I subclass your class. It may not have initialized its fields by the time they are required.

class BetterValueHolder extends ValueHolder
{
    private int betterValue;

    BetterValueHolder(final int betterValue)
    {
        // not actually required, it's added implicitly anyway.
        // just to demonstrate where your constructor is called from
        super();

        try
        {
            Thread.sleep(1000); // just to demonstrate the race condition more clearly
        }
        catch (InterruptedException e) {}

        this.betterValue = betterValue;
    }

    @Override
    public int getValue()
    {
        return betterValue;
    }
}

This will print zero, regardless of what value is given to the constructor of BetterValueHolder.

Does std::thread constructor completion actually synchronize with the beginning of the thread of execution?

Reading it, I thought the constructor completes before the start of the new thread

"synchronizes with" is a term of art. When the standard mandates that two operations synchronize with each other, that carries with it certain requirements for evaluations before and after the two operations. For example, accessing a variable in the original thread before the std::thread constructor, and accessing it in the new thread do not cause a data race.

Intuitively, you can think of "synchronizes with" as meaning that the new thread can see all prior evaluations, modifications, and side effects from the initial thread.

There is no need to make sure the thread begins by the end of the constructor. That is not what this says.

The way standard libraries enforce this requirement is by relying on underlying libraries like pthreads that essentially also enforce this requirement.

How to use a thread inside a class constructor in C++?

What you're describing is a worker thread that will run in the background. I wouldn't start a new thread every time you call WriteMessage() as thread creation is fairly expensive, and starting and stopping your thread can actually slow down your program. Instead you can start a thread in the constructor of the class and let it monitor a queue. Other clients of your Logger class can use the WriteMessage() function to push something onto the queue. The logger will detect some job has arrived and process it. At the end when you're finished call a Stop() function to stop the thread.

To do all this your thread has to execute a function that runs a loop. You can use a condition variable to wait on a condition, i.e. a job request or stop command. The advantage of a condition variable is that all the thread synchronization is done for you. You just have to specify the condition as a predicate. Putting something on the queue would have to be an atomic operation. You can use a std::lock_guard for that.

You can call other functions in Logger from the main thread while the worker thread sits in the background doing its job. That's not a problem.

Here's an implementation of this Logger class:

#include <thread>
#include <mutex>
#include <condition_variable>
#include <chrono>
#include <iostream>
#include <queue>

class Logger // singleton class
{
public:
    Logger() : mThread{}, mCV{}, mMutex{}, mQueue{}, mStop{ false }
    {
        mThread = std::thread(&Logger::Run, this); //create thread and execute Run()
    }

    ~Logger()
    {
        //Join thread
        if (mThread.joinable())
        {
            mThread.join();
        }
    }

    static Logger& getInstance() {
        static Logger logger;
        return logger;
    }

    void Stop()
    {
        {
            //Set the stop flag
            std::lock_guard<std::mutex> lk(mMutex);
            mStop = true;
        }
        mCV.notify_one();
    }

    void WriteMessage(const std::string& msg)
    {
        {
            //Push a request on the queue
            std::lock_guard<std::mutex> lk(mMutex);
            mQueue.push(msg);
        }
        mCV.notify_one();
    }

private:
    void Run()
    {
        while (true)
        {
            //Wait until some request comes or stop flag is set
            std::unique_lock<std::mutex> lock(mMutex);
            mCV.wait(lock, [&]() { return mStop || !mQueue.empty(); });

            //Stop if needed
            if (mStop)
            {
                break;
            }

            //Pop the job off the front of the queue
            std::string msg = std::move(mQueue.front());
            mQueue.pop();
            //Unlock the mutex
            lock.unlock();

            std::cout << msg << std::endl;
        }
    }

private:
    std::thread mThread;
    std::condition_variable mCV;
    std::mutex mMutex;
    std::queue<std::string> mQueue;
    bool mStop;
};

Working version here: https://ideone.com/wYIaMY

Program hanging after thread ending

the message "Ending Thread !" is executed normally , but the counter++ and the following println statement are never executed

So if new HandleConnection(client); actually starts a new thread (which you should not do in a constructor, see below), then the counter++ should immediately be executed and the "Number of clients... message printed. Any chance the message is appearing above the "Ending Thread!" message in your logs? Typically it takes some time to start the actual thread so the caller will continue to execute before the run() method is entered.

Other comments about your code:

• As @MarkoTopolnik mentions, you need to close the input and output streams in your run() method. finally clauses are a required pattern there.
• You should never call Thread.start() in an object constructor because of Thread race condition issues around object construction. See: Why not to start a thread in the constructor? How to terminate?

• Instead of extending Thread you should consider implementing Runnable and doing something like:

  new Thread(new HandleConnection(client)).start();
  

• Event better than managing the threads yourself would be to use an ExecutorService thread-pool for your client handlers. See this tutorial.

How to terminate all prior threads if a thread of the same type is run [JavaFX]

Below code launches a thread. Then, after a short pause (to simulate the first thread taking time to complete), a second thread is launched but before launching the second thread, the first thread is terminated.

public class Cesation implements Runnable {
    private volatile boolean quit;

    public synchronized void cease() {
        quit = true;
    }

    @Override
    public void run() {
        long total = Long.MIN_VALUE;
        while (!quit) {
            if (total == Long.MAX_VALUE) {
                total = Long.MIN_VALUE;
            }
            total++;
        }
        System.out.println("Final total = " + total);
    }

    public static void main(String[] args) {
        Cesation c1 = new Cesation();
        Thread t1 = new Thread(c1);
        t1.start();
        try {
            Thread.sleep(2000);
        }
        catch (InterruptedException x) {
            x.printStackTrace();
        }
        Cesation c2 = new Cesation();
        Thread t2 = new Thread(c2);
        c1.cease();
        try {
            t1.join();
        }
        catch (InterruptedException x1) {
            x1.printStackTrace();
        }
        t2.start();
        try {
            Thread.sleep(2000);
        }
        catch (InterruptedException x) {
            x.printStackTrace();
        }
        c2.cease();
    }
}

---

Related Topics