3
class MyClass
{
  private static volatile Resource resource;

  public static Resource getInstance()
  {
            if(resource == null)
                      resource = new Resource();
            return resource;
  }
}

Here if the Resource is a immutable class, is it safe to write the above code? As in java concurrency in practice it's mentioned that "initialization safety allow properly constructed immutable objects to be safely shared across thread.So the above code is safe to be written." (in page no 349 16.3). But with this it may possible if two threads checks for the null and they can proceed for object creation which is against the invariant of the class (singleton). please explain. A continuation of the question in link

Community
  • 1
  • 1
Trying
  • 14,004
  • 9
  • 70
  • 110
  • 2
    This question is *so* close to your [other question](http://stackoverflow.com/questions/15077910/singleton-with-volatile-in-java) it would take a hair to push it in the direction of being a duplicate. – Perception Feb 26 '13 at 00:27
  • @Perception Thanks. Are you saying that two threads will create two different objects? – Trying Feb 26 '13 at 00:28
  • Umm, no I'm not saying anything of the sort. I'm saying you could have made a slight amendment to your original question, to cover this edge scenario. – Perception Feb 26 '13 at 00:29
  • 2
    Why don't you just [use an enum singleton](http://stackoverflow.com/q/70689/139010) and be done with it? – Matt Ball Feb 26 '13 at 00:29
  • @MattBall you are right. But i just want to understand the inner concepts, thats all. Sorry if i am bothering you. – Trying Feb 26 '13 at 00:31

1 Answers1

6

No, this is not threadsafe code. In this case, Resource might be threadsafe, but your getInstance method is not.

Imagine this sequence of events

Thread1 calls getInstance and checks "if resource == null" and then stops (because the OS said it was time for it to be done) before initializing the resources.

Thread2 calls getInstance and checks "if resource == null" and then initializes the instance

Now Thread1 starts again and it also initializes the instance.

It has now been initialized twice and is not a singleton.

You have a couple of options to make it thread safe.

  1. Make the getInstance method synchronized

  2. Initialize the instance on declaration (or in a static initializer), and getInstance can just return it.

You also don't need to make the variable volatile. In case #1, synchronizing the method flushes the variable anyway so all variables will see an updated copy. In case #2, the object is guaranteed to be visible to all objects after construction.

Jeff Storey
  • 56,312
  • 72
  • 233
  • 406
  • then what is the meaning of the statement thats is given in the java concurrency in practice (in page no 349 16.3).Its mentioned that the above code is actually safe if resource is immutable. – Trying Feb 26 '13 at 00:36
  • That refers to #2. If you construct an immutable object (e.g. call its constructor), all threads are guaranteed to see the fully constructed instance (without making it volatile). In your case though, the issue is that you open up to the possibility of initializing an object twice, a different issue. – Jeff Storey Feb 26 '13 at 00:38
  • Thanks a lot for ur comment. That means u r saying in the book they are just talking about visibility problem but we need to solve here the atomicity problem as well, so synchronized is needed. Am i correct? I am just starting with the concurrency so sometimes my questions may seem silly. – Trying Feb 26 '13 at 00:41
  • `volatile` is necessary for double-checked locking, which in theory may be faster than synchronizing the entire method but in practice almost certainly won't matter. – Matt Ball Feb 26 '13 at 01:13
  • @MattBall but this isn't double checked locking – Jeff Storey Feb 26 '13 at 01:23