Skip to end of metadata
Go to start of metadata

To avoid data corruption in multithreaded Java programs, shared data must be protected from concurrent modifications and accesses. Locking can be performed at the object level using synchronized methods, synchronized blocks, or the java.util.concurrent dynamic lock objects. However, excessive use of locking can result in deadlocks.

Java neither prevents deadlocks nor requires their detection [JLS 2015]. Deadlock can occur when two or more threads request and release locks in different orders. Consequently, programs are required to avoid deadlock by acquiring and releasing locks in the same order.

Additionally, synchronization should be limited to cases where it is absolutely necessary. For example, the paint(), dispose(), stop(), and destroy() methods should never be synchronized in an applet because they are always called and used from dedicated threads. Furthermore, the Thread.stop() and Thread.destroy() methods are deprecated (see THI05-J. Do not use Thread.stop() to terminate threads for more information).

This rule also applies to programs that need to work with a limited set of resources. For example, liveness issues can arise when two or more threads are waiting for each other to release resources such as database connections. These issues can be resolved by letting each waiting thread retry the operation at random intervals until they successfully acquire the resource.

Noncompliant Code Example (Different Lock Orders)

This noncompliant code example can deadlock because of excessive synchronization. The balanceAmount field represents the total balance available for a particular BankAccount object. Users are allowed to initiate an operation that atomically transfers a specified amount from one account to another.

final class BankAccount {
  private double balanceAmount;  // Total amount in bank account

  BankAccount(double balance) {
    this.balanceAmount = balance;
  }

  // Deposits the amount from this object instance
  // to BankAccount instance argument ba
  private void depositAmount(BankAccount ba, double amount) {
    synchronized (this) {
      synchronized (ba) {
        if (amount > balanceAmount) {
          throw new IllegalArgumentException(
               "Transfer cannot be completed"
          );
        }
        ba.balanceAmount += amount;
        this.balanceAmount -= amount;
      }
    }
  }

  public static void initiateTransfer(final BankAccount first,
    final BankAccount second, final double amount) {

    Thread transfer = new Thread(new Runnable() {
        public void run() {
          first.depositAmount(second, amount);
        }
    });
    transfer.start();
  }
}

Objects of this class are prone to deadlock. An attacker who has two bank accounts can construct two threads that initiate balance transfers from two different BankAccount object instances a and b. For example, consider the following code:

BankAccount a = new BankAccount(5000);
BankAccount b = new BankAccount(6000);
BankAccount.initiateTransfer(a, b, 1000); // starts thread 1
BankAccount.initiateTransfer(b, a, 1000); // starts thread 2

Each transfer is performed in its own thread. The first thread atomically transfers the amount from a to b by depositing it in account b and then withdrawing the same amount from a. The second thread performs the reverse operation; that is, it transfers the amount from b to a. When executing depositAmount(), the first thread acquires a lock on object a. The second thread could acquire a lock on object b before the first thread can. Subsequently, the first thread would request a lock on b, which is already held by the second thread. The second thread would request a lock on a, which is already held by the first thread. This constitutes a deadlock condition because neither thread can proceed.

This noncompliant code example may or may not deadlock, depending on the scheduling details of the platform. Deadlock occurs when (1) two threads request the same two locks in different orders, and (2) each thread obtains a lock that prevents the other thread from completing its transfer. Deadlock is avoided when two threads request the same two locks but one thread completes its transfer before the other thread begins. Similarly, deadlock is avoided if the two threads request the same two locks in the same order (which would happen if they both transfer money from one account to a second account) or if two transfers involving distinct accounts occur concurrently.

Compliant Solution (Private Static Final Lock Object)

This compliant solution avoids deadlock by synchronizing on a private static final lock object before performing any account transfers:

final class BankAccount {
  private double balanceAmount;  // Total amount in bank account
  private static final Object lock = new Object();

  BankAccount(double balance) {
    this.balanceAmount = balance;
  }

  // Deposits the amount from this object instance
  // to BankAccount instance argument ba
  private void depositAmount(BankAccount ba, double amount) {
    synchronized (lock) {
      if (amount > balanceAmount) {
        throw new IllegalArgumentException(
            "Transfer cannot be completed");
      }
      ba.balanceAmount += amount;
      this.balanceAmount -= amount;
    }
  }

  public static void initiateTransfer(final BankAccount first,
    final BankAccount second, final double amount) {

    Thread transfer = new Thread(new Runnable() {
        @Override public void run() {
          first.depositAmount(second, amount);
        }
    });
    transfer.start();
  }
}

In this scenario, deadlock cannot occur when two threads with two different BankAccount objects try to transfer to each other's accounts simultaneously. One thread will acquire the private lock, complete its transfer, and release the lock before the other thread can proceed.

This solution imposes a performance penalty because a private static lock restricts the system to performing transfers sequentially. Two transfers involving four distinct accounts (with distinct target accounts) cannot be performed concurrently. This penalty increases considerably as the number of BankAccount objects increase. Consequently, this solution fails to scale well.

Compliant Solution (Ordered Locks)

This compliant solution ensures that multiple locks are acquired and released in the same order. It requires a consistent ordering over BankAccount objects. Consequently, the BankAccount class implements the java.lang.Comparable interface and overrides the compareTo() method.

final class BankAccount implements Comparable<BankAccount> {
  private double balanceAmount;  // Total amount in bank account
  private final Object lock;

  private final long id; // Unique for each BankAccount
  private static final AtomicLong nextID = new AtomicLong(0); // Next unused ID

  BankAccount(double balance) {
    this.balanceAmount = balance;
    this.lock = new Object();
    this.id = nextID.getAndIncrement();
  }

  @Override public int compareTo(BankAccount ba) {
     return (this.id > ba.id) ? 1 : (this.id < ba.id) ? -1 : 0;
  }

  // Deposits the amount from this object instance
  // to BankAccount instance argument ba
  public void depositAmount(BankAccount ba, double amount) {
    BankAccount former, latter;
    if (compareTo(ba) < 0) {
      former = this;
      latter = ba;
    } else {
      former = ba;
      latter = this;
    }
    synchronized (former) {
      synchronized (latter) {
        if (amount > balanceAmount) {
          throw new IllegalArgumentException(
              "Transfer cannot be completed");
        }
        ba.balanceAmount += amount;
        this.balanceAmount -= amount;
      }
    }
  }

  public static void initiateTransfer(final BankAccount first,
    final BankAccount second, final double amount) {

    Thread transfer = new Thread(new Runnable() {
        @Override public void run() {
          first.depositAmount(second, amount);
        }
    });
    transfer.start();
  }
}

Whenever a transfer occurs, the two BankAccount objects are ordered so that the first object's lock is acquired before the second object's lock. When two threads attempt transfers between the same two accounts, they each try to acquire the first account's lock before acquiring the second account's lock. Consequently, one thread acquires both locks, completes the transfer, and releases both locks before the other thread can proceed.

Unlike the previous compliant solution, this solution permits multiple concurrent transfers as long as the transfers involve distinct accounts.

Compliant Solution (ReentrantLock)

In this compliant solution, each BankAccount has a java.util.concurrent.locks.ReentrantLock. This design permits the depositAmount() method to attempt to acquire the locks of both accounts, to release the locks if it fails, and to try again later if necessary.

final class BankAccount {
  private double balanceAmount;  // Total amount in bank account
  private final Lock lock = new ReentrantLock();
  private final Random number = new Random(123L);

  BankAccount(double balance) {
    this.balanceAmount = balance;
  }

  // Deposits amount from this object instance
  // to BankAccount instance argument ba
  private void depositAmount(BankAccount ba, double amount)
                             throws InterruptedException {
    while (true) {
      if (this.lock.tryLock()) {
        try {
          if (ba.lock.tryLock()) {
            try {
              if (amount > balanceAmount) {
                throw new IllegalArgumentException(
                    "Transfer cannot be completed");
              }
              ba.balanceAmount += amount;
              this.balanceAmount -= amount;
              break;
            } finally {
              ba.lock.unlock();
            }
          }
        } finally {
          this.lock.unlock();
        }
      }
      int n = number.nextInt(1000);
      int TIME = 1000 + n; // 1 second + random delay to prevent livelock
      Thread.sleep(TIME);
    }
  }

  public static void initiateTransfer(final BankAccount first,
    final BankAccount second, final double amount) {

    Thread transfer = new Thread(new Runnable() {
        public void run() {
          try {
            first.depositAmount(second, amount);
          } catch (InterruptedException e) {
            Thread.currentThread().interrupt(); // Reset interrupted status
          }
        }
    });
    transfer.start();
  }
}

Deadlock is impossible in this compliant solution because locks are never held indefinitely. If the current object's lock is acquired but the second lock is unavailable, the first lock is released and the thread sleeps for some specified amount of time before attempting to reacquire the lock.

Code that uses this locking strategy has behavior similar to that of synchronized code that uses the traditional monitor lock. ReentrantLock also provides several other capabilities. For example, the tryLock() method immediately returns false when another thread already holds the lock. Further, the java.util.concurrent.locks.ReentrantReadWriteLock class has multiple-readers/single-writer semantics and is useful when some threads require a lock to write information while other threads require the lock to concurrently read the information.

Noncompliant Code Example (Different Lock Orders, Recursive)

The following immutable WebRequest class encapsulates a web request received by a server:

// Immutable WebRequest
public final class WebRequest {
  private final long bandwidth;
  private final long responseTime;

  public WebRequest(long bandwidth, long responseTime) {
    this.bandwidth = bandwidth;
    this.responseTime = responseTime;
  }

  public long getBandwidth() {
    return bandwidth;
  }

  public long getResponseTime() {
    return responseTime;
  }
}

Each request has a response time associated with it, along with a measurement of the network bandwidth required to fulfill the request.

This noncompliant code example monitors web requests and provides routines for calculating the average bandwidth and response time required to serve incoming requests.

public final class WebRequestAnalyzer {
  private final Vector<WebRequest> requests = new Vector<WebRequest>();

  public boolean addWebRequest(WebRequest request) {
    return requests.add(new WebRequest(request.getBandwidth(),
                        request.getResponseTime()));
  }

  public double getAverageBandwidth() {
    if (requests.size() == 0) {
      throw new IllegalStateException("The vector is empty!");
    }
    return calculateAverageBandwidth(0, 0);
  }

  public double getAverageResponseTime() {
    if (requests.size() == 0) {
      throw new IllegalStateException("The vector is empty!");
    }
    return calculateAverageResponseTime(requests.size() - 1, 0);
  }

  private double calculateAverageBandwidth(int i, long bandwidth) {
    if (i == requests.size()) {
      return bandwidth / requests.size();
    }
    synchronized (requests.elementAt(i)) {
      bandwidth += requests.get(i).getBandwidth();
      // Acquires locks in increasing order
      return calculateAverageBandwidth(++i, bandwidth);
    }
  }

  private double calculateAverageResponseTime(int i, long responseTime) {
    if (i <= -1) {
      return responseTime / requests.size();
    }
    synchronized (requests.elementAt(i)) {
      responseTime += requests.get(i).getResponseTime();
      // Acquires locks in decreasing order
      return calculateAverageResponseTime(--i, responseTime);
    }
  }
}

The monitoring application is built around the WebRequestAnalyzer class, which maintains a list of web requests using the requests vector and includes the addWebRequest() setter method. Any thread can request the average bandwidth or average response time of all web requests by invoking the getAverageBandwidth() and getAverageResponseTime() methods.

These methods use fine-grained locking by holding locks on individual elements (web requests) of the vector. These locks permit new requests to be added while the computations are still underway. Consequently, the statistics reported by the methods are accurate when they return the results.

Unfortunately, this noncompliant code example is prone to deadlock because the recursive calls within the synchronized regions of these methods acquire the intrinsic locks in opposite numerical orders. That is, calculateAverageBandwidth() requests locks from index 0 up to requests.size() − 1, whereas calculateAverageResponseTime() requests them from index requests.size() − 1 down to 0. Because of recursion, previously acquired locks are never released by either method. Deadlock occurs when two threads call these methods out of order, because one thread calls calculateAverageBandwidth(), while the other calls calculateAverageResponseTime() before either method has finished executing.

For example, when there are 20 requests in the vector, and one thread calls getAverageBandwidth(), the thread acquires the intrinsic lock of WebRequest 0, the first element in the vector. Meanwhile, if a second thread calls getAverageResponseTime(), it acquires the intrinsic lock of WebRequest 19, the last element in the vector. Consequently, deadlock results because neither thread can acquire all of the locks required to proceed with its calculations.

Note that the addWebRequest() method also has a race condition with calculateAverageResponseTime(). While iterating over the vector, new elements can be added to the vector, invalidating the results of the previous computation. This race condition can be prevented by locking on the last element of the vector (when it contains at least one element) before inserting the element.

Compliant Solution

In this compliant solution, the two calculation methods acquire and release locks in the same order, beginning with the first web request in the vector.

public final class WebRequestAnalyzer {
  private final Vector<WebRequest> requests = new Vector<WebRequest>();

  public boolean addWebRequest(WebRequest request) {
    return requests.add(new WebRequest(request.getBandwidth(),
                        request.getResponseTime()));
  }

  public double getAverageBandwidth() {
    if (requests.size() == 0) {
      throw new IllegalStateException("The vector is empty!");
    }
    return calculateAverageBandwidth(0, 0);
  }

  public double getAverageResponseTime() {
    if (requests.size() == 0) {
      throw new IllegalStateException("The vector is empty!");
    }
    return calculateAverageResponseTime(0, 0);
  }

  private double calculateAverageBandwidth(int i, long bandwidth) {
    if (i == requests.size()) {
      return bandwidth / requests.size();
    }
    synchronized (requests.elementAt(i)) {
      // Acquires locks in increasing order
      bandwidth += requests.get(i).getBandwidth();
      return calculateAverageBandwidth(++i, bandwidth);
    }
  }

  private double calculateAverageResponseTime(int i, long responseTime) {
    if (i == requests.size()) {
      return responseTime / requests.size();
    }
    synchronized (requests.elementAt(i)) {
      // Acquires locks in increasing order
      responseTime += requests.get(i).getResponseTime();
      return calculateAverageResponseTime(++i, responseTime);
    }
  }
}

Consequently, while one thread is calculating the average bandwidth or response time, another thread cannot interfere or induce deadlock. Each thread must first synchronize on the first web request, which cannot happen until any prior calculation completes.

Locking on the last element of the vector in addWebRequest() is unnecessary for two reasons. First, the locks are acquired in increasing order in all the methods. Second, updates to the vector are reflected in the results of the computations.

Risk Assessment

Acquiring and releasing locks in the wrong order can result in deadlock.

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

LCK07-J

Low

Likely

High

P3

L3

Automated Detection

Some static analysis tools can detect violations of this rule.

ToolVersionCheckerDescription
Coverity7.5

LOCK_INVERSION
LOCK_ORDERING

Implemented
Parasoft Jtest 10.3 TRS.LORDImplemented
ThreadSafe1.3

CCE_DL_DEADLOCK

Implemented

 

Related Guidelines

Bibliography

 


26 Comments

    • Compliant solution needs a bit of background discussion explaining what it does. The discussion about locking is good. Also we traditionally present the noncompliant solution first, as the noncompliant solution reflects common (insecure) practice.
    • The title should be more specific, eg "Avoid deadlock by requesting fine-grained locks in the proper order"
  1. It would be nice if you could also supply the FundConstants interface.


    This one seems to fall under the realms of CON08-J. Do not call alien methods that synchronize on the same objects as any callers in the execution chain which is dedicated to deadlocks and misleading synchronization. In fact, it is pretty similar to the 3rd NCCE.

    The distinguishing factor is, this one ought to be a rule since it always results in a deadlock while CON00-J is a recommendation because it just shows how synchronization can be incorrectly implemented (it also discusses recursive locks whereas a programmer believes that the shared data is safe). Moreover, the 3rd NCCE in CON00-J does not cause a deadlock always so can still be preserved as a subtle condition.

    Thus, this shall be classified separately as a rule.

  2. I couldn't get past the first noncompliant example in this guideline. This example talks about an amount. It is unclear which value this refers to. The withdraw() method zeros out the account, which makes no sense. The description then talks about "transferred from a to b by first depositing the amount from a to b" which again makes no sense. This needs to be rewritten to be as clear as possible. Also calling the class BadLocks seems to make sense until this same identifier starts showing up in the compliant solution. Just call it whatever the programmer would call it in a normal application.

    1. I tried to address your comments by expanding the explanation. I was trying to figure out the optimal amount of text to include but I suspect it increases the burden on the reader. Hopefully, it's easier to follow now.

      I've completely replaced the second NCE/CS that the student added. Also, I just discovered that it was borrowed from this link. The example was too long and complicated.

      Any comments about the new examples? Thanks.

      1. Also, I reinstated the "debug statements" because they introduce a delay and allow one to confirm the deadlock behavior more easily. In their absence, the program appears to do nothing because there is no output as such. Keeping them is not mandatory though; what do you think?

      2. EDIT: I've reworded it a bit.

        In regards to your last edit:

        An attacker may cause the program to construct two threads so that they initiate balance transfers from the first object to the second object and from the second object to the first object.

        At the beginning of the paragraph the notion of the "two objects" is undefined (as opposed to "two threads". This is done in subsequent lines so it might be better to avoid using the words "first" and "second" in the opening line.

        1. Every object and every thread has a name, so my first preference was to reference everything by name. If you agree, please make this change.

          1. This is now fixed. Thanks.

  3. The 1st NCCE's deadlock has nothing to do with 'requesting locks in the proper order'. The solution does not involve specifying an order for locking objects; this seems to not be possible. The whole NCCE/CS belongs in some other rule about doing proper locking, or avoiding locks when possible.

    • The 1st NCCE has a race condition (we want no problems in the NCCE except deadlock). It violates VNA02-J. Ensure that compound operations on shared variables are atomic. In particular when t1 and t2 are trying to run depositAllAmount, t1 might get its value incremented by t2 before it sets its own value to 0, in which case this transferred money 'disappears'. To fix this, you need to have depositAllAmount synchronize on both this and ba. Which does not solve the deadlock problem, but solves the race condition. It also guarantees that when printed ba shows the same new value it was given when this transferred all its balance.
    • FTM I would separate out the main() function, as that is 'attacker code'.
    • Can we s/monitor/intrinsic lock/g? Alternately, can we add 'monitor' to the definitions. (I used to get infuriated that Java used 'monitor' in exception names w/o defining it in any of the 'intro to java' tutorials I read.)
    • The volatile CS solves the deadlock, but not the race condition. Needs reworked.

    The 2nd NCCE (distance/time/speed) is a better example of requesting locks in the correct order. But its also mathematically nonsensical. If I take two plane flights from NYC to LA via Chicago, my total speed is NOT speed(NYC->Chicago) + speed(Chicago+LAX). The only good thing about the NCCE/CS pair is that the locking mechanism correctly shows the problem of acquiring locks in the wrong order. So we need to keep the lock arrays, but revamp everything else.

      • I don't follow your comment:

        In particular when t1 and t2 are trying to run depositAllAmount, t1 might get its value incremented by t2 before it sets its own value to 0, in which case this transferred money "disappears".

      t1 is incrementing t2 before setting its own value to 0. Because the method is synchronized, it won't allow t2 to update t1 simultaneously. However, the race condition is that t2 can still be updated from other threads while this is going on and t1 will see a stale value.

      • We could get rid of the main() methods once we have consensus over the validity of the examples.
      • I think we could add monitor to the definitions because the JLS uses it abundantly.
      • The CS does violate CON01. We need a static lock object.

      Wrt the 2nd NCE, I assumed that we are traveling at constant speeds between distances. It clearly doesn't illustrate a real world scenario. Do you have a more concise idea to demonstrate this issue? I've updated it now to address your concern.

  4. If we choose to keep the racers example:

    • It should probably use an internal private lock object array
    • The return statements should be moved outside the synchronized blocks
    • We can eliminate the getAverage*() methods and pass the arguments to the calculators directly.
      • I changed the 'racers' array to be private and final...that should be sufficient.
      • Moving the return statements outside the sync blocks would destroy the locking mechanism, rendering these useless as NCCEs or CSs
      • You could do that, the getAverage() methods are a simpler API, so I think they are worthwhile.
        • Class Racers would probably need to use synchronization/volatile for this to work. If you do that then you violate CON04 because Racers is public and it has synchronized methods. If someone instantiates Racer and locks on that object, then the methods of Racer cannot acquire the same lock causing a DoS.
        • I don't follow this. Why are you using synchronization for the whole operation? As soon as you get the distance/speed of the previous array element why do you need to hold the lock? Why do you want to ensure that at one time only averageDistanceCalculator() or averageSpeedCalculator() can run? Fine grained locking is almost always better.
        • I'll make this change later to reduce the size of the code snippets
        1. BTW, it appears that you cannot find the average distance and average speed at one particular point in the race. The method calls will need to be be performed atomically for that to happen.

          EDIT: To add, the Race class might be more functional if it's constructor accepts a racer[] and assigns it to the internal racers[] array instead of trying to initialize. Also, I think the CS won't compile.

            • I made the 'racers' array non-static, final, private, and initialized by a ctor argument. (To guarnatee thread-safety the caller must relinquish any references to the array.)
            • I've also made sure both the Race NCCE & CS compiles.

            While they lengthen the samples the getAverageCurrentSpeed() and getAverageDistance() do simplify the fact that (in the NCCE) one calls its recusrive algo with an arg of 0 and the other calls its recursive algo with an arg of MAX - 1. So I think its worthwhile leaving them in. I could see taking them out for illustration purposes, but I'd demand they be in if I were to use this class in production code (smile)

            1. I still don't follow why you are locking on racers. If you need to determine the statistics at a particular point in the race, why not use a single lock? For example, suppose the average speed is being calculated and before the computation is over, entries at the end of the racers[] array may change. As there are no setters to change racers[], I assume the only way this class is thread-unsafe is that the client may tweak the values of its own racers[] after passing it to the Race constructor and because this class Race does not defensively copy the racers[], its own internal references will change.

              Also note that we require deep copying in the Race constructor because we are not copying just primitives, but Racer objects (which are perhaps designed to be mutable?). I am also worried about the design of the Racer class because it has no setters. Perhaps it needs a constructor that takes two arguments speed and distance. It may also pay to make the fields in Racer final to make it immutable. This will avoid the need to make deep copies in the Race class (shallow copies are still required). If you do all this, there isn't much left in using synchronized(racers[i]){ }.

              The example in version 73 justified the synchronization because there was a complex interplay between different internal arrays such as time[] and speed[]. Threads invoking different methods could pass different values either times[] or speeds[] and set the internal arrays so that there are actual thread problems. The racers example is safe if you do not violate other guidelines such as defensive copying.

                • Added a defensive clone of the racers array to the Race ctor
                • Added setter methods to the Race; you can set any racer's speed or distance. Grabs the racer's lock while setting the stats.

                You can still argue that a single lock on all the racers would be better. I'd agree if there are a lot more 'average' calls than set calls, but if you have lots of 'average' calls and few 'set' calls, then this design is justified.

                1. At this point, it appears that version 73's code is shorter and simpler to understand w/o needing to justify about a single lock.

                  1. The code in v73 is only simpler b/c it doesn't have the features you requested.

                    • First off, the getAverageSpeed() is at best confusing, at worst mathematically wrong. (we've argued this before.
                    • There is no thread-safety b/c any class in the package can set any value in the array w/o locking anything.
                    • You can't change the size of the arrays or initialize them with current data.

                    These are surmountable (except the first), but fixing them beefs up the code size and complexity to match the Racer/Race code.

                    The Racer/Race code could be simplified in various ways to improve its illustrate-ability, but these all have costs in terms of good design. One idea would be to make Racer a static inner class of Race, which allows you to scrap its setter methods. Earlier versions of the Racer/Race example were simpler and left some things implicit (such as no defensive clone).

                    Bottom line: easy-to-understand, correct, short: pick two.

                      • It's not incorrect. Perhaps you recalled a previous version. v73 calculates average speed as sumof(all distances[])/sumof(all times[]).
                      • This is right and now we get into the rut of using a single lock instead of fine grained locking. I think I am convinced that neither example really motivates the need for fine grained locking.
                      • This can be fixed easily

                      With racers, I thin we could get rid of the ugly deep copying by making racers immutable (final fields set using a constructor). No need to even provide a clone method then. BTW, if you remove the setCurrentSpeed() and getAverageDistance() methods, you could just use a main() and make the example runnable.

                      I'll think more about the example(s) to motivate the need for fine-grained locking.

                      1. I think the functionality you want is better achieved with code like this. I will delete this comment later, however the CS should typically use code like this (NOTE: This code is not tested):

                        // Immutable Racer
                        public class Racer {
                          private final double currentSpeed;
                          private final double distanceTraveled; 
                        
                          public Racer(double speed, double distance) {
                            currentSpeed = speed;
                            distanceTraveled = distance;
                          }
                          
                          public double getCurrentSpeed() {
                            return currentSpeed;
                          }
                          
                          public double getDistance() {
                            return distanceTraveled;
                          }
                        }
                        

                        The Race class:

                        public final class RacerTest {
                          private final Vector<Racer> racers;
                          
                          public RacerTest(Vector<Racer> racer) {
                            racers = (Vector<Racer>) racer.clone(); 
                          }
                          
                          public boolean addRacer(Racer racer){
                            synchronized(racers) {
                              return racers.add(racer);
                            }
                          }
                          
                          public boolean removeRacer(Racer racer) {
                            synchronized(racers) {
                              return racers.remove(racer);  
                            }
                          }
                          
                          private double getAverageCurrentSpeed(int i, double currentSpeed) { 
                            if (i > racers.size()) {
                              return currentSpeed / racers.size();
                            }
                            currentSpeed += racers.get(i).getCurrentSpeed();
                            return getAverageCurrentSpeed(++i, currentSpeed);   	 	  
                          }
                        
                          private double getAverageCurrentDistance(int i, double distance) { 
                            if (i <= -1) {		 
                              return distance / racers.size();
                            }     
                            distance += racers.get(i).getDistance();
                            return getAverageCurrentDistance(--i, distance);	 
                          }
                          
                          public void getStatisticsAtSomePointInRace() {
                            synchronized(racers) {
                              getAverageCurrentSpeed(0, 0.0);
                              getAverageCurrentDistance(racers.size()-1, 0.0);
                            }
                          }
                          
                          public static void main(String[] args) {
                            Vector<Racer> racers = new Vector<Racer>();
                            for(int i=0;i<20;i++) {
                              racers.add(new Racer(1+i,2+i));
                            }	  
                          }
                        }
                        

                        The problem is we are trying to build an example by compromising better design techniques. The current CS although compliant with this guideline is probably not suitable to be used in a production environment. We still need to motivate the need for fine grained locks. For this, it might be preferable to have a shared array of primitives instead of a collection of objects.

                        1. This code shows no need to synchronize on the individual Racer objects, which was the whole point of the example.

                        2. Here's an idea: In the above example I suggested, we can say that the caller might want fine grained locking instead of using a single lock because we want to allow clients to add or remove racers even when the computation is going on (this avoids blocking for long periods and is a realistic scenario because cars can stall or start anytime). Then we can do away with synchronizing the adder (and synchronizing the remover so that removal is not performed when a computation has already acquired the corresponding racer's lock). Also, we use an internal lock so that getAvgCurrentSpeed() and getAvgCurrentDistance() are not called in different orders by threads.

                          Apart from this, my immutable Racer class should be declared final.

                          Here's the outline:

                          public final class RacerTest {
                            private final Vector<Racer> racers;
                            private final Object lock = new Object();
                            
                            public RacerTest(Vector<Racer> racer) {
                              racers = (Vector<Racer>) racer.clone(); 
                            }
                            
                            public boolean addRacer(Racer racer){	  
                              synchronized(racers.elementAt(racers.size()-1)) {
                                return racers.add(racer);
                              }
                            }
                            
                            public boolean removeRacer(Racer racer) {
                              synchronized(racers.elementAt(racers.indexOf(racer))) { 
                                return racers.remove(racer);      
                              }
                            }
                            
                            private double getAverageCurrentSpeed(int i, double currentSpeed) { 
                              if (i > racers.size()) {
                                return currentSpeed / racers.size();
                              }
                              synchronized(racers.elementAt(i)) {
                                currentSpeed += racers.get(i).getCurrentSpeed();
                                return getAverageCurrentSpeed(++i, currentSpeed);  
                              }
                            }
                          
                            private double getAverageCurrentDistance(int i, double distance) { 
                              if (i <= -1) {		 
                                return distance / racers.size();
                              }     
                              synchronized(racers.elementAt(i)) {
                                distance += racers.get(i).getDistance();
                                return getAverageCurrentDistance(--i, distance);
                              }
                            }
                            
                            public void getStatisticsAtSomePointInRace() {
                              synchronized(lock) {
                                getAverageCurrentSpeed(0, 0.0);
                                getAverageCurrentDistance(racers.size()-1, 0.0);
                              }
                            }
                            public static void main(String[] args) {
                              Vector<Racer> racers = new Vector<Racer>();
                              for(int i=0;i<20;i++) {
                                racers.add(new Racer(1+i,2+i));
                              }
                              Race race = new Race(racers);
                              race.getStatisticsAtSomePointInRace();    
                            }
                          }
                          

                          EDIT: Now the getAverageCurrentDistance() method can be safely called because I synchronized the addRacer() method on the last element of the vector.

  5.         } catch (InterruptedException e) {
              // Forward to handler
              Thread.currentThread().interrupt(); // Reset interrupted status
            }
    

    I don't think we need a 'forward to handler' here since we are clearly handling the InterruptedException and don't expect or want it to be logged (since we expect some other routine to handle the interrupt.

      1. Minor point: NextID's first letter should be lowercase. We should capitalize all letters of public static final fields (eg. singletons).