Thread-safe retirement account class

Are these classes properly guarded for thread safety? Do you see any issues with any of the classes?

@ThreadSafe
public class RetirementAccount {

     public static final int TYPE1 = 0;
     public static final int TYPE2 = 1;

    @GuardedBy("this")
    private final int accountType; 
    @GuardedBy("buySell")
    public final List buySell;

    public RetirementAccount(int accountType) {
       this.accountType = accountType;
       this.buySell = Collections.synchronizedList(new ArrayList());
    }

    public void addToAccount(double amount) {
       if (amount <= 0) {
            throw new IllegalArgumentException();
       } else {
           synchronized (buySell) { 
               buySell.add(new BuySell(amount));
            }
         }

      public double interestGained() {

           synchronized (this) { 
              switch (accountType) {
                …..
        }
      }

BuySell:

@Immutable
public class BySell {
    public final double depositAmount;

    public BuySell(double depositAmount) {
        this.depositAmount = depositAmount;

    }

  }

In your example code you have the following synchronization-related statements:

• this.buySell = Collections.synchronizedList(new ArrayList());

• synchronized (buySell) {

• synchronized (this) {

As a consequence, you are synchronizing on three different things in one class.

This is very confusing, and is buggy.

You are onlu showing us part of your class, but, there is no point in making buySell a synchronizedCollection if you are going to also gate access to it using synchronized(buySell).... it is redundant, and introduces 2 levels of synchronization.

Presumably when you calculate interest you also access the buySell class, and, as a consequence you have two levels of synchronization there too.

A deadlock is a situation where two threads each have a lock, and they each attempt to get another lock where that other lock is being held by the other thread. If you have multiple lock points in your class it becomes much easier to code things where you introduce deadlock possibilities. I cannot see any in your code at the moment, but your code is incomplete, and these are bugs that can creep in as the class is maintained.

The safest way to do things is to have just a single object which you use to cate access to your class. It is often tempting to use the actual instance itself as the gate, make every method synchronized, and, if you did, your class would become:

public class RetirementAccount {

    public static final int TYPE1 = 0;
    public static final int TYPE2 = 1;

    private final int accountType; 
    public final List buySell;

    public RetirementAccount(int accountType) {
       this.accountType = accountType;
       this.buySell = new ArrayList();
    }

   public synchronized void addToAccount(double amount) {
       if (amount <= 0) {
            throw new IllegalArgumentException();
       } else {
            buySell.add(new BuySell(amount));
       }
   }

   public synchronized double interestGained() {

        switch (accountType) {
                …..
   }

This would be a thread-safe class, and it has just one synchronization lock point (the class instance itself). There is no internal possibility of any deadlock, and it would be much better than what you have.

There is a better solution though....

The problem with the above solution is that you expose the locking system in the instance and make it public. Anyone can do:

synchronized(myaccount) {
    ....
}

and they essentially become a prt of your locking system, and that can result in lock-conditions that are unexpected when you created your class.

To make the class completely thread-safe, and have just a single lock point, and still keep isolated from the out-side, I recommend using a dedicated lock instance. It typically looks like this....

public class RetirementAccount {

    public static final int TYPE1 = 0;
    public static final int TYPE2 = 1;

    private final int accountType; 
    public final List buySell;
    private final Object synclock = new Object();

    public RetirementAccount(int accountType) {
       this.accountType = accountType;
       this.buySell = new ArrayList();
    }

   public void addToAccount(double amount) {
       synchronized (synclock) {
            if (amount <= 0) {
                throw new IllegalArgumentException();
           } else {
                buySell.add(new BuySell(amount));
           }
       }
   }

   public double interestGained() {

       synchronized (synclock) {

            switch (accountType) {
                …..
       }
   }

Synchronization and other locking mechanisms are complicated concepts, and they are hard to write, and hard to read. There are rules which make it easier:

• discipline - you have to create a system, and you have to follow it diligently

• dependence - use prefabricated classes where you can (java.util.concurrent.* is your friend) .... but understand what those classes do. Just because one instance is safe, it does not mean your system is safe. They do not solve all problems.

• discipline - you have to create a system, and you have to follow it diigently

• use as few locks as possible - the fewer locks you have to mentally track, the better.

• discipline - you have to create a system, and you have to follow it diigently

• avoid places where you have double-locking - you synchonize against one class, then synchronize against another. These double-locking situations are a precursor to deadlocks (they add risk).

• discipline - you have to create a system, and you have to follow it diigently

• avoid synchronizing classes with synchronized methods because that leads to conditions where outside influences can affect the internal locking of your instance.

• discipline - you have to create a system, and you have to follow it diigently

Did I mention that successful thread-safe implementations require discipline? You have to be diligent about implementing the synchronization correctly at each p

StackExchange Code Review Q#41852, answer score: 4