CODEWITHSUNDEEP
javathread-safety
Nirmalya
Nirmalya

Reputation: 25

Threads and synchronisation

I've problem understanding the following piece of code:-

public class SoCalledSigleton{

    private final static boolean allDataLoaded = SoCalledSigleton();

    private SoCalledSigleton(){

         loadDataFromDB();
         loadDataFromFile();
         loadDataAgainFromDB();

    }    
}

Is this piece of code thread safe? If not then Why?

Upvotes: 0

Views: 167

Answers (5)

Carl Smotricz
Carl Smotricz

Reputation: 67750

Yeah, it's thread safe. The "method" is the constructor, and it will be called when the class is loaded, i.e. exactly once.

But looking at the stuff being done, I think it's probably a lousy idea to call it from the class loader. Essentially, you'll end up doing your DB connection and stuff at the point in time when something in your code touches the SoCalledSingleton. Chances are, this will not be inside some well-defined sequence of events where, if there's an error you have catch blocks to take you to some helpful GUI message handling or whatever.

The "cleaner" way is to use a synchronized static getInstance() method, which will construct your class and call its code exactly when getInstance() is called the first time.

EDIT: As The Elite Gentleman pointed out, there's a syntax error in there. You need to say

private final static SoCalledSingleton allDataLoaded = new SoCalledSigleton();

Upvotes: 0

Andrzej Doyle
Andrzej Doyle

Reputation: 103777

From what we can see, there are no specific issues - it's guaranteed that the constructor will only ever by called once (so by definition can't be run multithreaded), which I presume is what you were concerned about.

However, there are still possible areas for problems. Firstly, if the loadData... methods are public, then they can be called by anyone at any time, and quite possibly could lead to concurrency errors.

Additionally, these methods are presumably modifying some kind of collection somewhere. If these collections are publically accessible before the constructor returns, then you can quite easily run into concurrency issues again. This could be an issue with anything exception updating instance-specific fields (static fields may or may not exhibit this problem depending where they are defined in the file).

Depending on the way the class is used, simply writing all of the data single-threaded may not be good enough. Collection classes are not necessarily safe for multi-threaded access even if read-only, so you'll need to ensure you're using the thread-safe data structures if multiple threads might access your singleton.

There are possibly other issues too. Thread-safety isn't a simple check-list; you need to think about what bits of code/data might be accessed concurrently, and ensure that appropriate action is taken (declaring methods synchronized, using concurrent collections, etc.). Thread-safety also isn't a binary thing (i.e. there's no such thing as "thread safe" per se); it depends how many threads will be accessing the class at once, what combinations of methods are thread-safe, whether sequences of operations will continue to function as one would expect (you can make a class "thread safe" in that is doesn't crash, but certain return values are undefined if pre-empted), what monitors threads need to hold to guarantee certain invariants etc.

I guess what I'm trying to say is that you need to think about and understand how the class is used. Showing people a snapshot of half a file (which doesn't even compile), and asking them to give a yes/no answer, is not going to be beneficial. At best they'll point out some of the issues for you if there are any; at worst you'll get a false sense of confidence.

Upvotes: 0

Péter Török
Péter Török

Reputation: 116246

(I assume that allDataLoaded is meant to be a SoCalledSigleton and boolean is just a typo :-)

If the class has no other constructors, or the loadData* methods don't do funny business (such as publishing this), its initialization is thread safe, because the initialization of final static data members is guarded by the JVM. Such members are initialized by the class loader when the class is first loaded. During this, there is a lock on the class so the initialization process is thread safe even if multiple threads try to access the class in parallel. So the constructor of the class is guaranteed to be called only once (per classloader - thanks Visage for the clarification :-).

Note that since you don't show us the rest of the class (I suppose it should have at least a static getInstance method, and probably further nonstatic members), we can't say anything about whether the whole implementation of the class is thread safe or not.

Upvotes: 2

Buhake Sindi
Buhake Sindi

Reputation: 89169

This will create an error in Java.

private final static boolean allDataLoaded = SoCalledSigleton();
  • You're assigning an object to a boolean variable.
  • You forgot to add new to instantiate the variable.

But if your code is like this

 public class SoCalledSigleton{

    private final static SoCalledSigleton allDataLoaded = new SoCalledSigleton();

    private SoCalledSigleton(){

         loadDataFromDB();
         loadDataFromFile();
         loadDataAgainFromDB();

    }    
}

It is thread-safe as static initialization and static attributes are thread-safe. They are initialized only once and exists throughout the whole life-cycle of the system.

Upvotes: 5

pauljwilliams
pauljwilliams

Reputation: 19225

The code is unusable in its current form, so any notions of thread safety are irrelevent.

What public interface would users use to get an instance of the singleton?

Upvotes: 2

Related Questions