Problem
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> buySell;
public RetirementAccount(int accountType) {
this.accountType = accountType;
this.buySell = Collections.synchronizedList(new ArrayList<BuySell>());
}
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;
}
}
Solution
In your example code you have the following synchronization-related statements:
this.buySell = Collections.synchronizedList(new ArrayList<BuySell>());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> buySell;
public RetirementAccount(int accountType) {
this.accountType = accountType;
this.buySell = new ArrayList<BuySell>();
}
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> buySell;
private final Object synclock = new Object();
public RetirementAccount(int accountType) {
this.accountType = accountType;
this.buySell = new ArrayList<BuySell>();
}
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 place it is needed.
Finally, there are ways you can force thread-safe operation with light versions of locking. My general advice to people is “don’t try to be clever unless you really are that clever”. It takes a special kind of brain to understand all the nuances of Java’s memory moela nd the way the threading affects it. If you have it clear in your mind, then sure, play some tricks…. but, even those who have i clear, mess up when they try to be clever.
Not a guru in the Concurrency yet, but I’ll try to be helpful.
First thing I notice is that in the addToAccount method you synchronize the access to the buySell list, which is redundant because it’s already synchronized in the constructor. The only time you would do that is when iterating through that list, because iterators are not synchronized. You can see that from the source code, compared with the collection’s add method
public boolean add(E e) {
synchronized (mutex) { return c.add(e); } // Synchronized
}
public Iterator<E> iterator() {
return c.iterator(); // Must be manually synched by user!
}
public ListIterator<E> listIterator() {
return list.listIterator(); // Must be manually synched by user
}
public ListIterator<E> listIterator(int index) {
return list.listIterator(index); // Must be manually synched by user
}
Moreover, I would get rid of the else statement
public void addToAccount(double amount) {
if (amount <= 0) {
throw new IllegalArgumentException();
}
buySell.add(new BuySell(amount));
}
The second thing is the interestGained method. You omitted that part but I suspect that you do use the buySell list in that switch statement. With the current implementation there could be a situation when the list is updated while the gained interest being calculated. If you want to avoid such situations you would probably synchronize on the buySell list instead of this, especially with the accountType being final.
The BuySell class is immutable, so it’s thread-safe by definition.
About the name of BuySell, from Clean Code by Robert C. Martin, page 25:
Classes and objects should have noun or noun phrase names like
Customer,WikiPage,
Account, andAddressParser. […] A class name should not be a verb.
Actually, what it contains is usually called Transaction. I’d rename the buySell field too to transactions.