Java concurrency in practice Listing10.6 synchronized

Question

Listing from jcip 10.6

 @ThreadSafe 
 class Taxi { 
 @GuardedBy("this") private Point location, destination; 
 private final Dispatcher dispatcher; 
 ... 
 public synchronized Point getLocation() { 
 return location; 
 }      
 public synchronized void setLocation(Point location) { 
 boolean reachedDestination; 
 synchronized (this) { 
 this.location = location; 
 reachedDestination = location.equals(destination); 
 } 
 if (reachedDestination) 
 dispatcher.notifyAvailable(this); 
 } 
} 

@ThreadSafe 
class Dispatcher { 
 @GuardedBy("this") private final Set<Taxi> taxis; 
 @GuardedBy("this") private final Set<Taxi> availableTaxis; 
 ... 
 public synchronized void notifyAvailable(Taxi taxi) { 
 availableTaxis.add(taxi); 
 } 

 public Image getImage() { 
 Set<Taxi> copy; 
 synchronized (this) { 
 copy = new HashSet<Taxi>(taxis); 
 } 
 Image image = new Image(); 
 for (Taxi t : copy) 
 image.drawMarker(t.getLocation()); 
 return image; 
 } 
}

My question is: Class Taxi, method setLocation used synchronized by "this" twice: first in method call, and second inside method. Is first synchronized redundant?

Method getLocation is synchronized also. For what purpose is it synchronized? Since set reference is atomic in java i think there is impossible to return corrupt location value. So i think synchronized on getLoction is also redundant.

One more question: If I make location valotile, is it acceptible to get rid of synchronized in getLocation?

thanks.

Answer 1

This must have been a mistake in previous prints, my version of JCIP 10.6 has this setLocation() method :

public void setLocation(Point location) {
    boolean reachedDestination;
    synchronized (this) {
        this.location = location;
        reachedDestination = location.equals(destination);
    }
    if (reachedDestination)
        dispatcher.notifyAvailable(this);
}

without method level synchronized.

Synchronization on the getter is needed to ensure it does not return a stale value.

Answer 2

See http://jcip.net.s3-website-us-east-1.amazonaws.com/errata.html

p.214: In Listing 10.6, Taxi.setLocation should not be a synchronized method. (The synchronized block in its body is correct, however.)

Answer 3

If setLocation has a method lock, I do think the inner lock is redundant. Probably, the method lock should not be there.

However, getLocation does need a lock, for memory visibility purposes. In particular, if getLocation were not synchronized, then updates done to location by setLocation may not be visible to a different thread, and getLocation could return a previous value for location.