CODEWITHSUNDEEP
multithreadingdesign-patternsjava-8singleton
ANKIT SRIVASTAVA
ANKIT SRIVASTAVA

Reputation: 173

Is this singleton implementation correct?

As the double lock checking doesn't work on optimized compilers I am removing it from my singleton class and instead of going for early initialization.

Below is my new singleton class : 

class MySingleton {
    private static volatile MySingleton myInstance = new MySingleton();
    public static getInstance() {
        return myInstance;
    }
}

Apart from getInstance(), there are setter methods in the class which set the values of the member fields.

Hope this implementation will not cause any data inconsistency when multiple threads will be updating various member fields using the same object.

Any suggestions or inputs are welcome.

Upvotes: 0

Views: 87

Answers (2)

Stephen C
Stephen C

Reputation: 718758

This code is thread-safe. Indeed, you can remove the volatile and it will still be thread-safe.

The myInstance variable is initialized when static initialization is triggered (if it hasn't already happened) by the first getInstance call. There is a happens-before from the initialization and the first call.

However, you also said this:

Hope this implementation will not cause any data inconsistency when multiple threads will be updating various member fields using the same object.

You haven't shown any of the code for doing that, so we can't tell you whether that code is thread-safe. If MySingleton instances are mutable, then the state-related methods must synchronize appropriately to prevents memory hazards and race conditions. The code that you have shown us does not address this.

Since the default MySingleton constructor is public, this is not a proper singleton class. Multiple instances can be created.

This implementation differs from many other implementations in that the distinguished MySingleton instance is created eagerly rather than lazily. This simplifies the problem considerably.

Note that lazy initialization of singletons is often unnecessary. If you don't specifically need lazy initialization, then the simpler eager alternative is just fine.

Upvotes: 3

Matt Timmermans
Matt Timmermans

Reputation: 59144

Your singleton is thread-safe, but there are things other than a call to getInstance() which could cause the MySingleton class to be initialized, which would construct the instance.

The usual way around this is to put the singleton in a private nested class like this:

class MySingleton {
    private MySingleton() {
        // there can only be one
    }
    private static class Holder {
        static final MySingleton instance = new MySingleton();
    }
    public static MySingleton getInstance() {
        return Holder.instance;
    }
}

Now the instance can only be constructed when the Holder class is initialized, and that only happens in the first call to getInstance()

Upvotes: 0

Related Questions