This is a poll of sorts about common concurrency problems in Java. An example might be the classic deadlock or race condition or perhaps EDT threading bugs in Swing. I'm interested both in a breadth of possible issues but also in what issues are most common. So, please leave one specific answer of a Java concurrency bug per comment and vote up if you see one you've encountered.
Source: Tips4all
The most common concurrency problem I've seen, is not realizing that a field written by one thread is not guaranteed to be seen by a different thread. A common application of this:
ReplyDeleteclass MyThread extends Thread {
private boolean stop = false;
public void run() {
while(!stop) {
doSomeWork();
}
}
public void setStop() {
this.stop = true;
}
}
As long as stop is not volatile or setStop is not synchronized this is not guaranteed to work. This mistake is especially devilish as in 99.999% it won't matter in practice as the reader thread will eventually see the change - but we don't know how soon he saw it.
My #1 most painful concurrency problem ever occurred when two different open source libraries did something like this:
ReplyDeleteprivate static final String LOCK = "LOCK"; // use matching strings
// in two different libraries
public doSomestuff() {
synchronized(LOCK) {
this.work();
}
}
At first glance, this looks like a pretty trivial synchronization example. However; because Strings are interned in Java, the literal string "LOCK" turns out to be the same instance of java.lang.String (even though they are declared completely disparately from each other.) The result is obviously bad.
One classic problem is changing the object you're synchronizing on while synchronizing on it:
ReplyDeletesynchronized(foo) {
foo = ...
}
Other concurrent threads are then synchronizing on a different object and this block does not provide the mutual exclusion you expect.
A common problem is using classes like Calendar and SimpleDateFormat from multiple threads (often by caching them in a static variable) without synchronization. These classes are not thread-safe so multi-threaded access will ultimately cause strange problems with inconsistent state.
ReplyDeleteDouble-Checked Locking. By and large.
ReplyDeleteThe paradigm, which I started learning the problems of when I was working at BEA, is that people will check a singleton in the following way:
public Class MySingleton {
private static MySingleton s_instance;
public static MySingleton getInstance() {
if(s_instance == null) {
synchronized(MySingleton.class) { s_instance = new MySingleton(); }
}
return s_instance;
}
}
This never works, because another thread might have gotten into the synchronized block and s_instance is no longer null. So the natural change is then to make it:
public static MySingleton getInstance() {
if(s_instance == null) {
synchronized(MySingleton.class) {
if(s_instance == null) s_instance = new MySingleton();
}
}
return s_instance;
}
That doesn't work either, because the Java Memory Model doesn't support it. You need to declare s_instance as volatile to make it work, and even then it only works on Java 5.
People that aren't familiar with the intricacies of the Java Memory Model mess this up all the time.
Though probably not exactly what you are asking for, the most frequent concurrency-related problem I've encountered (probably because it comes up in normal single-threaded code) is a
ReplyDeletejava.util.ConcurrentModificationException
caused by things like:
List<String> list = new ArrayList<String>(Arrays.asList("a", "b", "c"));
for (String string : list) { list.remove(string); }
Not properly synchronizing on objects returned by Collections.synchronizedXXX(), especially during iteration or multiple operations:
ReplyDeleteMap<String, String> map = Collections.synchronizedMap(new HashMap<String, String>());
...
if(!map.containsKey("foo"))
map.put("foo", "bar");
That's wrong. It should be:
synchronized(map) {
if(!map.containsKey("foo"))
map.put("foo", "bar");
}
Or with a ConcurrentMap implementation:
map.putIfAbsent("foo", "bar");
The most common bug we see where I work is programmers perform long operations, like server calls, on the EDT, locking up the GUI for a few seconds and making the app unresponsive.
ReplyDeleteAnother common bug is poor exception handling. When a background thread throws an exception, if you don't handle it properly, you might not see the stack trace at all. Or perhaps your background task stops running and never starts again because you failed to handle the exception.
ReplyDeleteForgetting to wait() (or Condition.await()) in a loop, checking that the waiting condition is actually true. Without this, you run into bugs from spurious wait() wakeups. Canonical usage should be:
ReplyDeletesynchronized (obj) {
while (<condition does not hold>) {
obj.wait();
}
// do stuff based on condition being true
}
It can be easy to think synchronized collections grant you more protection than they actually do, and forget to hold the lock between calls. If have seen this mistake a few times:
ReplyDeleteList<String> l = Collections.synchronizedList(new ArrayList<String>());
String[] s = l.toArray(new String[l.size()]);
For example, in the second line above, the toArray and size() methods are both thread safe in their own right, but the size() is evaluated separately from the toArray(), and the lock on the List is not held between these two calls. If you run this code with another thread concurrently removing items from the list, sooner or later you will end up with a new String[] returned which is larger than required to hold all the elements in the list, and has null values in the tail. It is easy to think that because the two method calls to the List occur in a single line of code this is somehow an atomic operation, but it is not.
Until I took a class with Brian Goetz I didn't realize that the non-synchronized getter of a private field mutated through a synchronized setter is never guaranteed to return the updated value. Only when a variable is protected by synchronized block on both reads AND writes will you get the guarantee of the latest value of the variable.
ReplyDeletepublic class SomeClass{
private Integer thing = 1;
public synchronized void setThing(Integer thing)
this.thing = thing;
}
/**
* This may return 1 forever and ever no matter what is set
* because the read is not synched
*/
public Integer getThing(){
return thing;
}
}
Thinking you are writing single-threaded code, but using mutable statics (including singletons). Obviously they will be shared between threads. This happens surprisingly often.
ReplyDeleteMy biggest problem has always been deadlocks, especially caused by listeners that are fired with a lock held. In these cases, it's really easy to get inverted locking between two threads. In my case, between a simulation running in one thread and a visualization of the simulation running in the UI thread.
ReplyDeleteEDIT: Moved second part to separate answer.
Arbitrary method calls should not be made from within synchronized blocks.
ReplyDeleteDave Ray touched on this in his first answer, and in fact I also encountered a deadlock also having to do with invoking methods on listeners from within a synchronized method. I think the more general lesson is that method calls should not be made "into the wild" from within a synchronized block - you have no idea if the call will be long-running, result in deadlock, or whatever.
In this case, and usually in general, the solution was to reduce the scope of the synchronized block to just protect a critical private section of code.
Also, since we were now accessing the Collection of listeners outside of a synchronized block, we changed it to be a copy-on-write Collection. Or we could have simply made a defensive copy of the Collection. The point being, there are usually alternatives to safely access a Collection of unknown objects.
I believe in the future the main problem with Java will be the (lack of) visibility guarantees for constructors. For example, if you create the following class
ReplyDeleteclass MyClass {
public int a = 1;
}
and then just read the value MyClass.a from another thread, MyClass.a could be either 0 or 1, depending on the JavaVM's implementation and mood. Today the chances for 'a' being 1 are very high. But on future NUMA machines this may be different. Many people are not aware of this and believe that they don't need to care about multi-threading during the initialization phase.
Mutable classes in shared data structures
ReplyDeleteThread1:
Person p = new Person("John");
sharedMap.put("Key", p);
assert(p.getName().equals("John"); // sometimes passes, sometimes fails
Thread2:
Person p = sharedMap.get("Key");
p.setName("Alfonso");
When this happens, the code is far more complex that this simplified example. Replicating, finding and fixing the bug is hard. Perhaps it could be avoided if we could mark certain classes as immutable and certain data structures as only holding immutable objects.
Not exactly a bug but, the worst sin is providing a library you intend other people to use, but not stating which classes/methods are thread-safe and which ones must only be called from a single thread etc.
ReplyDeleteMore people should make use of the concurrency annotations (e.g. @ThreadSafe, @GuardedBy etc) described in Goetz's book.
The most recent Concurrency-related bug I ran into was an object that in its constructor created an ExecutorService, but when the object was no longer referenced, it had never shutdown the ExecutorService. Thus, over a period of weeks, thousands of threads leaked, eventually causing the system to crash. (Technically, it didn't crash, but it did stop functioning properly, while continuing to run.)
ReplyDeleteTechnically, I suppose this isn't a concurrency problem, but it's a problem relating to use of the java.util.concurrency libraries.
Starting a thread within the constructor of a class is problematic. If the class is extended, the thread can be started before subclass' constructor is executed.
ReplyDeleteMultiple objects that are lock protected but are commonly accessed in succession. We've run into a couple of cases where the locks are obtained by different code in different orders, resulting in deadlock.
ReplyDeleteUnbalanced synchronization, particularly against Maps seems to be a fairly common problem. Many people believe that synchronizing on puts to a Map (not a ConcurrentMap, but say a HashMap) and not synchronizing on gets is sufficient. This however can lead to an infinite loop during re-hash.
ReplyDeleteThe same problem (partial synchronization) can occur anywhere you have shared state with reads and writes however.
I encountered a concurrency problem with Servlets, when there are mutable fields which will be setted by each request.
ReplyDeleteBut there is only one servlet-instance for all request, so this worked perfectly in a single user environment but when more than one user requested the servlet unpredictable results occured.
public class MyServlet implements Servlet{
private Object something;
public void service(ServletRequest request, ServletResponse response)
throws ServletException, IOException{
this.something = request.getAttribute("something");
doSomething();
}
private void doSomething(){
this.something ...
}
}
Using a local "new Object()" as mutex.
ReplyDeletesynchronized (new Object())
{
System.out.println("sdfs");
}
This is useless.
Another common 'concurrency' issue is to use synchronized code when it is not necessary at all. For example I still see programmers using StringBuffer or even java.util.Vector (as method local variables).
ReplyDeleteSynchronizing on a string literal or constant defined by a string literal is (potentially) a problem as the string literal is interned and will be shared by anyone else in the JVM using the same string literal. I know this problem has come up in application servers and other "container" scenarios.
ReplyDeleteExample:
private static final String SOMETHING = "foo";
synchronized(SOMETHING) {
//
}
In this case, anyone using the string "foo" to lock on is sharing the same lock.
The dumbest mistake I frequently make is forgetting to synchronize before calling notify() or wait() on an object.
ReplyDelete