methods in a singleton class are thread-safe?

Question

I'm designing a custom logging framework for our application.

I'm reading Patterns For Logging Diagnostic Messages, which is being used in http://logging.apache.org/log4j/1.2/apidocs/org/apache/log4j/NDC.html

On page 3 it says:

Because it is a Singleton, it easily preserves the order of the messages.

I think, for example, given Singleton class S, if a class B is trying to acquire the instance of S while a class A already got the instance S, then B cannot acquire the instance of S since A already got the instance of S.

That's why the order of message is preserved based on my understanding.

1. Is my understanding correct?

2. How the class B knows that class A is done with the class S and no longer needs it so that B can acquire S?

3. if my understanding is correct and if Singleton class S has some methods: test1() and test2() as you see below.

Are test1() and test2() are thread-safe?

These methods will be called outside of class S, something like

S.getInstance().test1("message")

in class A or class B for example.

Which means when class A and class B are trying to write some data in a log file by calling test1(), this data will be written in the order of acquiring the instance of S?

If not, to make methods thread-safe in a Singleton class S, should I also use synchronized keyword on methods test1() and test2() or lock for these methods?

public class S {                  // singleton class S

    private static S instance;

    private S(){}

    public static S getInstance(){
        if(instance == null){
            instance = new S();
        }
        return instance;
    }

    public static void test1(String log) {
       // writing some data to a log file
    }

    public static void test2(String log) {
       // writing some data to a log file
    }
}

Answer 1

This is definitely not thread-safe. Suppose for instance I had two threads T1, and T2, and S had property foo. Suppose T1 and T2 are modifying the value of foo, and then using the value of foo to perform some other operation.

Then, I could presumably have T1 access S.getInstance, check the getInstance is not set, and at the same time, T2 can access S.getInstance and see that the instance is not set. T1, could then potentially set the instance, but since T2 had also at the same time detected that the instance was not set, would also set the instance for S. Therefore, the value of S.instance, is going to actually be the one set by T2. In otherwords, there is a race condition between T1 and T2 to see who can set the instance of S first.

To make this synchronous, you should definitely have the getInstance method be synchronized so only one thread could be acting on it at once. Also, you should probably make the instance of S volatile so as to ensure that any thread that is accessing the instance of S is always going to be working with the "latest" copy. (because presumably one thread could be doing some other read operation on that instance while it's being modified).

i.e. something like this:

public class S {                  // singleton class S

    private volatile static S instance;

    private S(){}

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

    public static void test1(String log) {
       // writing some data to a log file
    }

    public static void test2(String log) {
       // writing some data to a log file
    }
}

Also, here's a good link on why you should use volatile:

What is the point of making the singleton instance volatile while using double lock?

Answer 2

The code in your example is not thread safe. If two concurrent threads try to get the singleton object at the same time, there is a chance each thread creates an object. You should use synchronized keyword on the getInstance method to avoid this behavior.

You also have to mark as synchronized every instance method (not static) on the singleton class that access data from the object (non-static properties). This way, two different methods will never run concurrently, preventing the data from being messed up.

In your example, you don't need to use synchronized on test1 and test2 as long as the methods they call from the instance are all synchronized.

Your S class should probably look like this

public class S {

    private static S instance;

    private S() {}

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

    public synchronized doSomething() {
        // Code that uses data from instance
    }

    public static void test1(String log) {
        // ...
        S.getInstance().doSomthing();
        // ...
    }

    public static void test2(String log) {
        // ...
        S.getInstance().doSomthing();
        // ...
    }
}

Answer 3

First, your code sample above is not a safe singleton. There are possibility of race conditions at static method getInstance. If 2 or more threads run if(instance==null) at the same time, more than one S instances will be constructed. To correct this use eager-initialization by providing a static final field of your class instance.

Example:-

public class S{

     private static final S INSTANCE = new S();

     private S(){}

     public static S getInstance(){
         return INSTANCE;
     }

     public void doSomething(){
         //the rest of the code
     }
}

Even better since JDK 5, use enum type to represent safe singleton. It also provide guards against serialization attack for free.

Example:-

public enum Singleton {

    S;

    public void doSomething(){/*...*/}
}

Secondly, if doSomething does not modify S instance state or it is a stateless object, then it is thread-safe. Otherwise have to provide synchronization guards to preserve its state correctness in multi-threaded environment.

Note: In the past, many naive implementations of double locking idiom to solve lazy-loading singleton without synchronization were fraught with perils. Refer to good article below signed by Doug Lee, Joshua Bloch & et al. for further read.

The "Double-Checked Locking is Broken" Declaration

Answer 4

To answer your questions:

1. No, your understanding is incorrect.
2. By using some sort of locking mechanism, such as a Semaphore.
3. That depends entirely on what's inside those methods. If they only use variables that are local to the method (i.e. those that are on the stack and not in the heap) that the method is probably thread-safe. If you need to access to variables that are local to the class, such as a log file, you again need a locking mechanism.

Just to add, you cannot have a static getInstance method and expect it to be thread-safe. The best way to accomplish this is to have code as follows:

private final static S INSTANCE = new S();

This will only be instantiated the moment you first access it.

Answer 5

This is not thread safe. Imagine two competing threads trying to get an instance of S, and both checking at the same time if the instance is null. In order to lock this for thread safety you will need to use the synchronized keyword. Try this:

public class S {                  // singleton class S

    private static S instance;

    private S(){}

    public static S getInstance(){
        if(instance == null){
            synchronized (S.class)
            {
                if(instance == null)
                {
                    instance = new S();
                }
            }

        }
        return instance;
    }

    public static void test1(String log) {
        // writing some data to a log file
    }

    public static void test2(String log) {
        // writing some data to a log file
    }
}

Another option, to avoid the double-checked locking above would be to synchronize the getInstance method like so (although with and added performance hit):

public static synchronized S getInstance()
    {
        if (instance == null)
        {
            instance = new S();
        }

        return instance;
    }