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?