This specific example?
Threadingwise it's fine. Style wise it's deplorable. Do NOT write catch blocks like that. It also means if an exception does occur (it can't here - your constructor is empty), your code will dump half of the info to system error, and then continue, with a null reference instance of an instance of Singleton - causing other code to spit out NullPointerExceptions (because code just keeps going, as you caught the exception instead of letting it happen). If you treat all exceptions in this fashion, a single error will cause hundreds of errors in your logs, all irrelevant except the first one.
Once you take care of this exception handling issue, you can make the variable final
, and no longer assign null to it. While you're at it, make the whole class final
. It effectively is already (as you only have a private constructor):
public final class Singleton {
private static final Singleton INSTANCE = new Singleton();
private Single() {}
public static Singleton getInstance() {
return INSTANCE;
}
The reason this works out when 2 threads simultaneously invoke getInstance, is the classloader mechanism itself: The Classloader guarantees that any given class is never loaded more than once by the same loader, even if 2 threads would simultaneously require this (the classloader will synchronize/lock to avoid this situation), and the initialization process (the static block - which was needlessly convoluted, as the example above shows) is similarly guarded and cannot possibly occur twice.
That's the only freebie you get: For static methods as a general rule, all threads can just run the same method all simultaneously if they want to. And here they do - it's just that the initialization (which includes the ... = new Singleton();
part) is gated to occur only once.
NB: If you must do more complex things, make helper methods:
public final class Singleton {
private static Singleton INSTANCE = create();
private Singleton(Data d) {
// do stuff with 'd'
}
private static Singleton create() {
Data d;
try {
d = readStuffFromDataIncludedInMyJar();
} catch (IOException e) {
throw new Error("states.txt is corrupted", e);
}
return new Singleton(d);
}
}
This:
- Keeps code simple - static initializers are a thing, but fairly exotic java.
- Makes your code easier to test.
- It's an internal file; if that is missing/broken, that's about as likely / as problematic as one of your class files having gone for a walk. An Error is warranted here. This cannot possibly occur unless you wrote a bug or messed up a build, and hard crashing with a clear exception telling you precisely what's wrong is exactly what you want to happen in that case, not for code to blindly continue in a state where half of your app is overwritten with gobbledygook due to a disk drive crash or what not. Best to just conclude everything's borked, say so, and stop running.