A fellow developer in our group asked me an interesting question about the Singleton Design Pattern. I have a Singleton class and I am posting the pertinent code below:
The idea is to generate a Singleton instance. He is proposing a simpler solution and asking if "lazy-loading" is not needed then why is the following code not thread-safe and why will it not return a single instance?
I cannot answer his question to his satisfaction. Any ideas?
I believe that the above code will not generate a "single" instance but as many instances as the number of times the "DataSchema.getInstance()" static method is called from the client code, and therefore, as many threads will be accessing the ONE data-schema underlying these instances of the DataSchema class.
If I am wrong, please explain why. If I am right, I need to word it in a better way which I am having trouble with.
There is a chance that it may not return a single instance in some cases. I think the safe way to do that (and this is what I always use) is this:
The trick here being that static classes are always loaded once, and only once.
Joined: Jul 11, 2001
Sorry, you're both wrong.
First, the original code, which uses the double checked locking pattern, is *not* threadsafe, as explained by the link provided by Brian.
Second, the alternative presented in the original post *is* threadsafe. A class is guaranteed to be only loaded and initialized once, and to be fully initialized before used by any thread, so the static field will only be initialized once and than be accesible by all threads without any problems.
And third, the solution offered by Brian will work, but is not necessary in this case. There *is* a similar solution with which you can use an inner class to implement thread safe *lazy* initialization without use of synchronization, as far as I remember. Most often it's simply not worth the effort, though.
Hope this helps...
The soul is dyed the color of its thoughts. Think only on those things that are in line with your principles and can bear the light of day. The content of your character is your choice. Day by day, what you do is who you become. Your integrity is your destiny - it is the light that guides your way. - Heraclitus
Joined: Jul 11, 2001
Moving to Threads and Synchronization, by the way...
I noticed that this question has detoured into the "double check locking" problem... so I'll address the question first.
Either solution, "initiatizing as a static variable", or "lazy initiation" will both work, and is thread safe (okay, just in case, the variable should be declared volatile). The issue has more to do with timing. Generally, you want to do lazy initialization if you need to delay the initialization to when you actually needed. This is important if there are resource limitations, or side effect issues. For example, I need to connect to a service. It needs to be singleton as all threads can share the resource. But it is also expensive to use, so I don't want to connect unless I absolutely need to.
As for the "double check locking" problem, it is not that it is not thread safe. It is a problem because it just doesn't work. Newer JVMs, and even some older ones, will code motion safe code into a synchronized block for efficiency. Since the code outside the block is just a "if" statement, on a variable, it will most likely be moved into the block. The code will still work, and is threadsafe, but it defeats the original purpose of why you did it -- and that is to avoid synchronization.
Henry [ April 15, 2005: Message edited by: Henry Wong ]
Originally posted by Ilja Preuss: Sorry, you're both wrong. ... And third, the solution offered by Brian will work, but is not necessary in this case. There *is* a similar solution with which you can use an inner class to implement thread safe *lazy* initialization without use of synchronization, as far as I remember. Most often it's simply not worth the effort, though.
Hope this helps...
The solution offered by Brian works and is necessary if you want lazy loading. Else if you use the 2nd solution you get initialization when the class is loaded and not when the method is called. The 3rd solution is more lazy
The trouble with the out of order writes (allocating the memory and then executing the constructor) is that the normal double checked locking checks the object against null if (instance == null). That might cause timing issues and returning an object that has not been correctly initialized. but here we check a Boolean (boolean assignments are atomic). In this scenario the Boolean created is set to true INSIDE of the sync block. The first thread exiting the sync block will flush properly the created (setting it to true). So if the second thread passed the first check and got blocked because the first one is in the sync block, it will not pass the second check though. I don�t see how a compiler could optimize the code above? In the listing 7 of the first link I mentioned above there is a broken �fix� for the double checked idiom because of the JIT compiler optimization. But the solutions are different. Compare:
It seems obvious that the compiler would optimize it (move the line 5 inside of the sync block).
Where is the trouble in my snippet?
Joe [ May 13, 2005: Message edited by: Joe Boxer ]
[ May 13, 2005: Message edited by: Joe Boxer ] [ May 13, 2005: Message edited by: Joe Boxer ]
Joined: Mar 04, 2004
[I]What about this?
Why is this still broken?[/I]
Because the "if (created == false)" block is outside the synchronized block, the synchronized block could be skipped but a null pointer returned.
I don�t see how a compiler could optimize the code above?
A modern processor could speculatively execute the "return _me" statement before testing the value of "created" - perhaps "created" needs to be fetched from main memory, but "_me" does not, because it's already in cache.
After the speculative return of the (null) "_me" value, but before "created" is checked, another thread, perhaps running on a different processor, creates the singleton object. The first thread now finds that "created" is true, and thus the speculatively returned null value becomes the actual return value.
Joined: Oct 23, 2003
Thanks for your explanation about the optimization, it stands to reason. I can imagine that due to the caching of variables a NULL object could be returned, if fetching of �created� will be executed later then return _me.
But why would the first line be skipped?
Under what circumstances could it happen? I mean, we write lines of code to be �executed� and not to hope that they �might be executed�. Is there a link/book you would recommend to read a bit more about that issue?
Actually, there is no way that the line can be skipped...
What can happen though, is that Java will "code motion" the "if" condition from outside of the block to inside the synchronized block. It will run exactly the same, and it will save on having to check the variable twice -- which should be a good thing.
However, the original reason why these hoops were jumped was to avoid unnecessary synchronization, which the code does not achieve... The code is not broken because it doesn't run correctly -- it does. It is broken because the side affect of avoiding synchronization is not achieved.
Henry [ May 15, 2005: Message edited by: Henry Wong ]
Joined: Nov 13, 2002
I'm still curious about the original code - why would they claim it doesn't work ? I mean, all static variables are guaranteed be initialized before invoking any methods (including getInstance())... isn't that true ? Is there an error in the question ?
Joined: Jul 11, 2001
Originally posted by Sol Mam-Orn: I mean, all static variables are guaranteed be initialized before invoking any methods (including getInstance())... isn't that true ?