Is this code Double Checked Locking safe?
-
29-10-2019 - |
Frage
I am looking at some code in our app that I think may be encountering a case of "Double-checked locking". I have written some sample code that is similar to what we do.
Can anyone see how this can be experiencing double-checked locking? Or is this safe?
class Foo {
private Helper helper = null;
public Helper getHelper() {
Helper result;
synchronized(this) {
result = helper;
}
if (helper == null) {
synchronized(this) {
if (helper == null) {
helper = new Helper();
}
}
}
return helper;
}
}
Base code borrowed from wiki.
Lösung
The whole point of double-checked locking is that the fast path (when you don't need to instantiate the object) isn't synchronized. So what you have isn't double-checked locking.
You need to get rid of the first synchronized block in order to get a broken double-checked lock. Then you need to make helper
volatile to fix it.
Andere Tipps
It's unnecessarily complex, the simplest 'safe' way to do DCL is like so:
class Foo {
private volatile Helper helper = null;
private final Object mutex = new Object();
public Helper getHelper() {
if (helper == null) {
synchronized(mutex) {
if (helper == null) {
helper = new Helper();
}
}
}
return helper;
}
}
The key points here being:
- In the 'happy' case we expect helper to already be assigned, so if it is we can just return it without having to enter a synchronized block.
- Helper is marked as volatile to let the compiler know that helper can be read from / written to by any thread at any time and it's important that read / writes are not re-ordered.
- The synchronized block uses a private final variable to synchronize on to avoid a potential performance hit in the case of another area of code synchronizing on the
this
instance.
This part is a standard double-checked locking:
if (helper == null) {
synchronized(this) {
if (helper == null) {
helper = new Helper();
}
}
}
The first part is a useless assignment that does nothing to the double-checked locking part: if helper is null then it is executed anyway, if it isn't it wouldn't be executed anyway. It's completely ineffective.