CODEWITHSUNDEEP
javadesign-patternssingleton
fedvasu
fedvasu

Reputation: 1252

mutating object as singleton?

I am not sure, if anyone has asked this question before (If so apologies).

code copied from wikipedia.

public class Singleton {
   private static final Singleton instance = new Singleton();

   private Singleton() {}

   public static Singleton getInstance() {
      return instance;
   }
}

How it is thread safe? is it(only) because there is no mutating state in this class?

what happens if I modify it like this 

public class Singleton {
    private static final Singleton instance = new Singleton();
    private FancyClass obj1;
    private FancyClass obj2;

    //feel free to imagine all the getters and setters for obj1 and obj2,
    // like getObj1() and so forth

    //tricky method
    public void doSomething() {
      obj1.destroyEnemy();
      obj2.destroyFriend();
    }

    private Singleton() {
       obj1 = null;
       obj2 = null;
    }

    public static Singleton getInstance() {
      return instance;
    }
}

I have no interest in design-patterns discussion, this is the kind of code, I am supposed to maintain. is the above 'singleton' thread safe, assuming that FancyClass is java standard library class.

Upvotes: 2

Views: 441

Answers (4)

Nathan Hughes
Nathan Hughes

Reputation: 96434

You're right that the first example is safe only because there is no mutable state.

In the second example, the singleton isn't doing anything to make it threadsafe, whether it's safe depends on if FancyClass is threadsafe. Putting synchronized on the doSomething method would make it threadsafe.

Introducing getters and setters for obj1 and obj2 would also have the potential to create problems, those would have to be synchronized on the same lock as the doSomething method, and you would need locking even if FancyClass is threadsafe, because if different threads are changing things then those changes need to be made visible across threads.

Upvotes: 3

Aniket Thakur
Aniket Thakur

Reputation: 68985

About your code - Since you are making your constructor private it will never be called. So declaring it null there makes no sense. Anyway instance variables are automatically assigned default values.

First of all the 1st code

   public class Singleton {
   private static final Singleton instance = new Singleton();

   private Singleton() {}

   public static Singleton getInstance() {
      return instance;
   }
}

is called Early initialization unlike lazy initialization in which you create instance only when needed. To make it thread safe you need to do the following

public class Singleton {
  private static Singleton singleInstance;
    private Singleton() {}
  public static Singleton getSingleInstance() {
    if (singleInstance == null) {
      synchronized (Singleton.class) {
        if (singleInstance == null) {
          singleInstance = new Singleton();
        }
      }
    }
    return singleInstance;
  }

Example is of lazy initialization but the concept remains the same.

Upvotes: 0

Andreas Dolk
Andreas Dolk

Reputation: 114817

The first code guarantees that all threads that call getInstance() get a reference to the same instance of Singleton.

For your implementation (second example), the same is true for obj1 and obj2, because they're created while the class itself is created and class creation is thread safe (can't be created twice/"in parallel" within the same classloader).

The doSomething() method is not thread safe. Make it synchronized if you need an atomic operation.

Upvotes: 3

gma
gma

Reputation: 2563

The Singleton pattern is not designed to be Thread Safe at all, just to be unique and forbidding creation of additional instances. The only part that is thread safe in you code is the Singleton instantiation private static final Singleton instance = new Singleton(); as it's invoked one time a class loading time.

But effectively, if there's no members in your class modified by a method invked concurently by external systems, your Singleton will be/stay ThreadSafe

Upvotes: 1

Related Questions