mutating object as singleton?

Question

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.

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. — Aniket Thakur, May 22 '13 at 14:15
@AniketThakur Yeah, That is super-dumb(initiializing in constructor with null). I was just rewriting an existing class without specifics. — fedvasu, May 22 '13 at 14:40
Note, bunging a `synchronized` in wont make operations that require multiple method calls thread-safe. A `synchronized` "get" method is usually pointless. (ObComment: Mutable statics are a really bad idea.) — Tom Hawtin - tackline, May 22 '13 at 15:28

Nathan Hughes · Accepted Answer · 2013-05-22T14:13:30.280

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.

Ok, FancyClass is thread safe according to docs. (part of JDK) — fedvasu, May 22 '13 at 14:09

score 3 · Answer 2 · answered May 22 '13 at 14:10

3

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.

answered May 22 '13 at 14:10

Andreas Dolk

113,398
19
180
268

I assume same would be true with, getters and setters of obj1 and obj2, I need to make them synchronized as well? – fedvasu May 22 '13 at 14:44
@fedvasu If you only set or get the objects, then synchronization is not necessary. See: http://stackoverflow.com/a/1351271/105224 – Andreas Dolk May 22 '13 at 17:14

score 1 · Answer 3 · answered May 22 '13 at 14:08

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

score 0 · Answer 4 · answered May 22 '13 at 14:12

0

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.

answered May 22 '13 at 14:12

Aniket Thakur

66,731
38
279
289

You forgot set it as volatile it should be: private static volatile Singleton singleInstance = null; – Andres L May 22 '13 at 14:16
Why do you need to if statements checking the same thing. I'm assuming that's a mistake? – Andrew Martin May 22 '13 at 14:18
@AndrewMartin No it is not a mistake. It is called double locking mechanism. Here is the [Wiki](http://en.wikipedia.org/wiki/Double-checked_locking) – Aniket Thakur May 22 '13 at 14:20
@AniketThakur: Thanks - never seen that before. Will read up on it. – Andrew Martin May 22 '13 at 14:22
@AndresL Yes problems might arise during thread scheduling. So it is good practice to declare it as volatile as well. Thanks for pointing it out. – Aniket Thakur May 22 '13 at 14:22

mutating object as singleton?

4 Answers4