Private constructor to avoid race condition
There are already a bunch of answers here, but I would really like to dive into some details (as much as my knowledge let's me). I will strongly advise you to run each sample that is present here in the answer to see for yourself how things are happening and why.
To understand the solution, you need to understand the problem first.
Suppose that the SafePoint class actually looks like this:
class SafePoint {
private int x;
private int y;
public SafePoint(int x, int y){
this.x = x;
this.y = y;
}
public SafePoint(SafePoint safePoint){
this(safePoint.x, safePoint.y);
}
public synchronized int[] getXY(){
return new int[]{x,y};
}
public synchronized void setXY(int x, int y){
this.x = x;
//Simulate some resource intensive work that starts EXACTLY at this point, causing a small delay
try {
Thread.sleep(10 * 100);
} catch (InterruptedException e) {
e.printStackTrace();
}
this.y = y;
}
public String toString(){
return Objects.toStringHelper(this.getClass()).add("X", x).add("Y", y).toString();
}
}
What variables create the state of this object? Just two of them : x,y. Are they protected by some synchronization mechanism? Well they are by the intrinsic lock, through the synchronized keyword - at least in the setters and getters. Are they 'touched' anywhere else? Of course here:
public SafePoint(SafePoint safePoint){
this(safePoint.x, safePoint.y);
}
What you are doing here is reading from your object. For a class to be Thread safe, you have to coordinate read/write access to it, or synchronize on the same lock. But there is no such thing happening here. The setXY method is indeed synchronized, but the clone constructor is not, thus calling these two can be done in a non thread-safe way. Can we brake this class?
Let's try this out:
public class SafePointMain {
public static void main(String[] args) throws Exception {
final SafePoint originalSafePoint = new SafePoint(1,1);
//One Thread is trying to change this SafePoint
new Thread(new Runnable() {
@Override
public void run() {
originalSafePoint.setXY(2, 2);
System.out.println("Original : " + originalSafePoint.toString());
}
}).start();
//The other Thread is trying to create a copy. The copy, depending on the JVM, MUST be either (1,1) or (2,2)
//depending on which Thread starts first, but it can not be (1,2) or (2,1) for example.
new Thread(new Runnable() {
@Override
public void run() {
SafePoint copySafePoint = new SafePoint(originalSafePoint);
System.out.println("Copy : " + copySafePoint.toString());
}
}).start();
}
}
The output is easily this one:
Copy : SafePoint{X=2, Y=1}
Original : SafePoint{X=2, Y=2}
This is logic, because one Thread updates=writes to our object and the other is reading from it. They do not synchronize on some common lock, thus the output.
Solution?
synchronized constructor so that the read will synchronize on the same lock, but Constructors in Java can not use the synchronized keyword - which is logic of course.
-
may be use a different lock, like Reentrant lock (if the synchronized keyword can not be used). But it will also not work, because the first statement inside a constructor must be a call to this/super. If we implement a different lock then the first line would have to be something like this:
lock.lock() //where lock is ReentrantLock, the compiler is not going to allow this for the reason stated above.
what if we make the constructor a method? Of course this will work!
See this code for example
/*
* this is a refactored method, instead of a constructor
*/
public SafePoint cloneSafePoint(SafePoint originalSafePoint){
int [] xy = originalSafePoint.getXY();
return new SafePoint(xy[0], xy[1]);
}
And the call would look like this:
public void run() {
SafePoint copySafePoint = originalSafePoint.cloneSafePoint(originalSafePoint);
//SafePoint copySafePoint = new SafePoint(originalSafePoint);
System.out.println("Copy : " + copySafePoint.toString());
}
This time the code runs as expected, because the read and the write are synchronized on the same lock, but we have dropped the constructor. What if this were not allowed?
We need to find a way to read and write to SafePoint synchronized on the same lock.
Ideally we would want something like this:
public SafePoint(SafePoint safePoint){
int [] xy = safePoint.getXY();
this(xy[0], xy[1]);
}
But the compiler does not allow this.
We can read safely by invoking the *getXY method, so we need a way to use that, but we do not have a constructor that takes such an argument thus - create one.
private SafePoint(int [] xy){
this(xy[0], xy[1]);
}
And then, the actual invokation:
public SafePoint (SafePoint safePoint){
this(safePoint.getXY());
}
Notice that the constructor is private, this is because we do not want to expose yet another public constructor and think again about the invariants of the class, thus we make it private - and only we can invoke it.
The private constructor is an alternative to:
public SafePoint(SafePoint p) {
int[] a = p.get();
this.x = a[0];
this.y = a[1];
}
but allows constructor chaining to avoid duplication of the initialization.
If SafePoint(int[]) were public then the SafePoint class couldn't guarantee thread-safety because the contents of the array could be modified, by another thread holding a reference to the same array, between the values of x and y being read by the SafePoint class.
Constructors in Java can not be synchronized.
We can not implement public SafePoint(SafePoint p) as { this (p.x, p.y); } because
As we are not synchronized(and can't as we are in the constructor),
during the execution of the constructor, someone may be calling SafePoint.set() from the different thread
public synchronized void set(int x, int y){
this.x = x; //this value was changed
--> this.y = y; //this value is not changed yet
}
so we will read the object in the inconsistent state.
So instead we create a snapshot in a thread-safe way, and pass it to the private constructor. The stack confinement protects the reference to the array, so there's nothing to worry about.
update
Ha! As for the trick everything is simple - you have missed @ThreadSafe annotation from the book in your example:
@ThreadSafe
public class SafePoint { }
so, if the constructor which takes int array as an argument will be public or protected, the class will no longer be thread-safe, because the content of the array may change the same way as the SafePoint class(i.e. someone may change it during the constructor execution)!
I understand that it provides a getter to retrieve both x and y at once in a array instead of a separate getter for each, so the caller will see consistent value, but why private constructor ? what's the trick here?
What we want here is chaining of constructor calls to avoid code duplication. Ideally something like this is what we want:
public SafePoint(SafePoint p) {
int[] values = p.get();
this(values[0], values[1]);
}
But that won't work because we will get a compiler error:
call to this must be first statement in constructor
And we can't use this either:
public SafePoint(SafePoint p) {
this(p.get()[0], p.get()[1]); // alternatively this(p.x, p.y);
}
Because then we have a condition where the values might have been changed between the call to p.get().
So we want to capture the values from SafePoint and chain to another constructor. That is why we will use the private constructor capture idiom and capture the values in a private constructor and chain to a "real" constructor:
private SafePoint(int[] a) {
this(a[0], a[1]);
}
Also note that
private SafePoint (int [] a) { this (a[0], a[1]); }
does not make any sense outside the class. A 2-D point has two values, not arbitrary values like the array suggests. It has no checks for the length of the array nor that it is not null. It is only used within the class and the caller knows it is safe to call with two values from the array.