Reputation: 14278
class MyClass
{
private static volatile Resource resource;
public static Resource getInstance()
{
if(resource == null)
resource = new Resource();
return resource;
}
}
Here my doubt is according to java concurrency in practice if you use volatile, safe publication happens (i.e. as soon the reference is visible to another thread the data is also available). So can I use it here? But if it is correct then suppose thread1 now checks "resource" and it's null so it starts creating the object. While thread1 is creating the objet another thread i.e. thread2 comes and start checking the value of "resource" and thread2 finds it as null (assume creating "resource" object takes some considerable amount of time and as thread1 has not yet completed the creation so the safe publication hasn't happened hence unavailable to thread2 )then will it also start creating the object? if yes then class invariant breaks. Am I correct? Please help me in understanding this specially use of volatile here.
Upvotes: 15
Views: 17913
Reputation: 2751
Here is my suggestion to add volatile & synchronized together.
Note: we still have to do double check.
public class MySingleton {
private static volatile MySingleton instance;
private MySingleton() {}
synchronized private static void newInstance() {
if(instance == null) {
instance = new MySingleton();
}
}
public static MySingleton get() {
if(instance == null) {
newInstance();
}
return instance;
}
}
Upvotes: 0
Reputation:
volatile solves one issue that is visibility issue . If you are writing to one variable that is declared volatile then the value will be visible to other thread immediately. As we all know we have different level of cache in os L1, L2, L3 and if we write to a variable in one thread it is not guaranteed to be visible to other, so if we use volatile it writes to direct memory and is visible to others. But volatile does not solve the issue of atomicity i.e. int a; a++;
is not safe. AS there are three machine instructions associated to it.
Upvotes: 16
Reputation: 302
first of all, having a Singleton this way, you are essentially creating a global object which is a bad practice. I reckon you use Enums instead.
Upvotes: 0
Reputation: 56479
I think, you should use syncronized
keyword before getInstance
definition.
For better performance you can use Double-checked locking pattern:
Upvotes: 1
Reputation: 40256
I know you aren't asking about better solutions but this is definitely worth if you are looking for a lazy singleton solution.
Use a private static class to load the singleton. The class isn't loaded until invocation and so the reference isn't loaded until the class is. Class loading by implementation is thread-safe and you also incur very little overhead (in case you are doing repetitive volatile loads [which may still be cheap], this resolution always normal loads after initial construction).
class MyClass {
public static Resource getInstance() {
return ResourceLoader.RESOURCE;
}
private static final class ResourceLoader {
private static final Resource RESOURCE = new Resource();
}
}
Upvotes: 7
Reputation: 8432
volatile
keyword guarantees that read and write to that variable are atomic.
According to the tutorial
Reads and writes are atomic for all variables declared volatile
Using volatile variables reduces the risk of memory consistency errors, because any write to a volatile variable establishes a happens-before relationship with subsequent reads of that same variable. This means that changes to a volatile variable are always visible to other threads. What's more, it also means that when a thread reads a volatile variable, it sees not just the latest change to the volatile, but also the side effects of the code that led up the change.
Upvotes: 0
Reputation: 2338
You are correct, in this case the Resource may be constructed twice due to the race you describe. If you want to implement a singleton (without explicit locking) in Java 5+, use an enum singleton as described in answers to What is an efficient way to implement a singleton pattern in Java?.
Upvotes: 0
Reputation:
When applied to a field, the Java volatile guarantees that:
(In all versions of Java) There is a global ordering on the reads and writes to a volatile variable. This implies that every thread accessing a volatile field will read its current value before continuing, instead of (potentially) using a cached value. (However, there is no guarantee about the relative ordering of volatile reads and writes with regular reads and writes, meaning that it's generally not a useful threading construct.)
(In Java 5 or later) Volatile reads and writes establish a happens-before relationship, much like acquiring and releasing a mutex.
More info.
Upvotes: 0
Reputation: 82559
You are correct, multiple threads could try to create a Resource object. Volatile just guarantees that if one thread updates the reference, all other threads will see the new reference, not some cached reference. This is slower, but safer.
If you require only a single resource that is lazy loaded, you need to do something like this:
class MyClass
{
private static volatile Resource resource;
private static final Object LOCK = new Object();
public static Resource getInstance()
{
if(resource == null) {
synchronized(LOCK) { // Add a synch block
if(resource == null) { // verify some other synch block didn't
// write a resource yet...
resource = new Resource();
}
}
}
return resource;
}
}
Upvotes: 19