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

Question

Possible Duplicate:
Java: Why not to start a thread in the constructor? How to terminate?

I'm used to run FindBugs on my code in order to find bugs or bad practice. Today it complains on the fact that I'm starting a thread in a class constructor.

Is really a bad thing to do? Could you explain me why?

It is at least safe if my class is final?

EDIT:

The thread is implemented as an inner class and it only uses fields of the main class already initialized when it is started:

public final class SingletonOuter {
    private static SingletonOuter ourInstance = new SingletonOuter();

    public static SingletonOuter getInstance() {
        return ourInstance;
    }

    private final SomeOtherClass aField;

    private SingletonOuter() {
        aField=new SomeOtherClass(); 
        thread=new InnerThread();
        thread.start();
    }

    private boolean pleaseStop;

    private synchronized boolean askedStop(){return pleaseStop;}
    public synchronized void stop(){
        pleaseStop=true;  
    }

    private final InnerThread thread ;
    private class InnerThread extends Thread{
        @Override public void run() {
            //do stuff with aField until askedStop()
        }
    }

}

EDIT:

I finally move the start of thread to the getInstance method, to avoid the possibility of introducing future bugs:

public final class SingletonOuter {
        private static SingletonOuter ourInstance

        public static SingletonOuter getInstance() {
            if (ourInstance==null){
                ourInstance= = new SingletonOuter();
                ourInstance.thread.start();
            }

            return ourInstance;
        }

        private final SomeOtherClass aField;

        private SingletonOuter() {
            aField=new SomeOtherClass(); 
            thread=new InnerThread();

        }
        ...

Answer 1

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.

Answer 2

In general, it is best to be gentle with what you do in a constructor.

Your object is still in an invalid state, so you don't want anybody to have access to it. When you start a thread from a constructor, it will likely have a reference to the object that is being constructed (otherwise why is the constructor starting it?). This reference will point to an invalid object when the thread starts and soon after it will become valid. There is an immediate potential for horrible race conditions there.

Here is a link to a good article about it http://www.ibm.com/developerworks/java/library/j-jtp0618/index.html

Answer 3

Every time you instantiate that class you create a thread. Threads are expensive and very difficult to test. If you instantiate many objects you will run into performance problems, you should consider a ThreadPool to fix a limit on the number of threads. Also, if you will have trouble trying to unit test whatever behavior is happening in the thread.