What is the most frequent concurrency issue you've encountered in Java?

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.

My #1 most painful concurrency problem ever occurred when two different open source libraries did something like this:

private 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.

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:

class 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 and run are 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.

One classic problem is changing the object you're synchronizing on while synchronizing on it:

synchronized(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.

Not properly synchronizing on objects returned by Collections.synchronizedXXX(), especially during iteration or multiple operations:

Map<String, String> map = Collections.synchronizedMap(new HashMap<String, String>());

...

if(!map.containsKey("foo"))
    map.put("foo", "bar");

That's wrong. Despite single operations being synchronized, state of map between invoking contains and put can be changed by another thread. It should be:

synchronized(map) {
    if(!map.containsKey("foo"))
        map.put("foo", "bar");
}

Or with a ConcurrentMap implementation:

map.putIfAbsent("foo", "bar");

Double-Checked Locking. By and large.

The 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

java.util.ConcurrentModificationException

caused by things like:

List<String> list = new ArrayList<String>(Arrays.asList("a", "b", "c"));
for (String string : list) { list.remove(string); }

It can be easy to think synchronized collections grant you more protection than they actually do, and forget to hold the lock between calls. I have seen this mistake a few times:

 List<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.

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.

Forgetting 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:

 synchronized (obj) {
     while (<condition does not hold>) {
         obj.wait();
     }
     // do stuff based on condition being true
 }

Another 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.

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.

public 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.

Arbitrary method calls should not be made from within synchronized blocks.

Dave 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.

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.)

Technically, I suppose this isn't a concurrency problem, but it's a problem relating to use of the java.util.concurrency libraries.

Unbalanced 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.

The 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. But 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 ...
    }
}

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.

More people should make use of the concurrency annotations (e.g. @ThreadSafe, @GuardedBy etc) described in Goetz's book.

My 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.

EDIT: Moved second part to separate answer.

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.

Mutable classes in shared data structures

Thread1:
    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.

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

class MyClass {
    public int a = 1;
}

and then just read the MyClass's property 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.

Synchronizing 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.

Example:

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.

Using a local "new Object()" as mutex.

synchronized (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).

Multiple 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.

Not realising that the this in an inner class is not the this of the outer class. Typically in an anonymous inner class that implements Runnable. The root problem is that because synchronisation is part of all Objects there is effectively no static type checking. I've seen this at least twice on usenet, and it also appears in Brian Goetz'z Java Concurrency in Practice.

BGGA closures don't suffer from this as there is no this for the closure (this references the outer class). If you use non-this objects as locks then it gets around this problem and others.

Use of a global object such as a static variable for locking.

This leads to very bad performance because of contention.

Honesly? Prior to the advent of java.util.concurrent, the most common problem I routinely ran into was what I call "thread-thrashing": Applications that use threads for concurrency, but spawn too many of them and end up thrashing.

Not realising the java.awt.EventQueue.invokeAndWait acts as if it holds a lock (exclusive access to the Event Dispatch Thread, EDT). The great thing about deadlocks is that even if that happens rarely you can grab a stack trace with jstack or the like. I've seen this in a number of widely used programs (a fix to a problem I have only seen occur once in Netbeans should be included in the next release).

1. A common mistake that I have encountered involves iterating over a synchronized Collection class. It is required to manually synchronized before getting the iterator and while iterating.

2. Another mistake is that most textbooks give the impression that making a class thread safe is just a matter of adding synchronized on every method. That in itself is not a guarantee - it will only protect the integrity of the particular class, but the results can still be undeterministic.

3. Putting too much time-costly operations in a synchronized block often result in very bad performance. Fortunately the Future pattern in the concurrency package can safe the day.

4. Caching mutable objects to improve performance often leads to multithreading issues as well (and sometimes very hard to track since you assume you are the only user).

5. Using multiple synchronisation objects must be carefully handled.

Starting Java RMI causes a background task to run that forces the garbage collector to run every 60 seconds. In itself, this may be a good thing, however it may be that the RMI server wasn't started by you directly, but by a framework/tool you use (eg. JRun). And, the RMI might not actually be being used for anything.

The net result is a System.gc() call once a minute. On a heavily loaded system, you will see the following output in your logs - 60 seconds of activity followed by a long gc pause followed by 60 seconds of activity followed by a long gc pause. This is fatal to throughput.

The solution is to turn off explicit gc using -XX:+DisableExplicitGC

Try this code..

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 ...
    }
}

Race conditions during an object's finalize/release/shutdown/destructor method and normal invocations.

From Java, I do a lot of integration with resources that need to be closed, such as COM objects or Flash players. Developers always forget to do this properly and end up having a thread call an object that has been shutdown.

Failure to provide clearly defined lifecycle methods on objects that manage long-running threads. I like to create pairs of methods named init() and destroy(). It is also important to actually call destroy() so your app can exit gracefully.

Keeping all threads busy.

This is most frequent with having to go fix problems in other people's code, because they abused the locking constructs. As of late, my coworkers seem to have found reader/writer locks quite fun to sprinkle around whereas a little thought removes their need entirely.

In my own code, keeping the threads busy is less obvious but challenging. It requires deeper thought into algorithms, such as writing new data structures, or carefully designing a system to ensure that when locking is used it will never be contended.

Solving concurrency mistakes is easy - trying to figure out how to avoid lock contention can be hard.

A method saving data to an instance variable in order to "save effort" passing it to helper methods, when another method which can be called concurrently uses the same instance variables for its own purposes.

The data should instead be passed around as method parameters for the duration of the synchronized call. This is only a slight simplification of my worst memory:

public class UserService {

    private String userName;

    public String getUserName() {
        return userName;
    }
  
    public void login(String name) {
        this.userName = name; 
        doLogin();
    }

    private void doLogin() {
        userDao.login(getUserName());
    }

    public void delete(String name) {
        this.userName = name; 
        doDelete();
    }

    private void doDelete() {
        userDao.delete(getUserName());
    }

}

The login and logout methods do not have to be synchronized, logically speaking. But written as-is you get to expeience all sorts of fun customer service calls.

Concurrency problem of using different lock objects with wait and notify.

I was trying to use wait() and notifyAll() methods and here is how i used and fell in hell.

Thread1

Object o1 = new Object();

synchronized(o1) {
    o1.wait();
}

And in other thread. Thread - 2

Object o2 = new Object();

synchronized(o2) {
    o2.notifyAll();
}

Thread1 will wait on o1 and Thread2 which should have invoked o1.notifyAll(), is invoking o2.notifyAll(). Thread 1 will never wake up.

And offcourse the common problem of not invoking wait() or notifyAll() within synchronized blocks and not invoking them using the same object that is used to sycnhronze the block.

Object o2 = new Object();

synchronized(o2) {
    notifyAll();
}

This will cause IllegalMonitorStateException, since the thread that invoked notifyAll() has invoked notifyAll() using this object but is not the owner of the this lock object. But the current thread is owner of o2 lock object.

The biggest problem I have run across is developers that add multi-threading support as an afterthought.

Since Java 5 there is Thread.getUncaughtExceptionHandler but this UncaughtExceptionHandler is never called when a ExecutorService/ThreadPool is used.
At least I was not able to get the UncaughtExceptionHandler with an ExcutorService working.

My two cents on trying to avoid synchronization problems from the start — watch out for the following issues/smells:

1. When writing code, always know in which thread you're in.
2. When designing a class or API for reuse, always ask yourself whether the code has to be thread-safe. It's better to make a deliberate decision, and document that your unit is not thread-safe, than to put in unwise synchronization with potential for deadlock.
3. Invocations of new Thread() are a smell. Use dedicated ExecutorServices instead, which force you to think about your application's overall threading concept (see 1) and encourage others to follow it.
4. Know and use library classes (like AtomicBoolean et al, synchronized Collections, etc). Again: make a conscious decision on whether thread-safety is important in a given context, don't just use them blindly.

I ran into a pseudo-deadlock from an I/O thread that created a countdown latch. A vastly simplified version of the problem is like:

public class MyReader implements Runnable {

private final CountDownLatch done = new CountDownLatch(1);
private volatile isOkToRun = true;
public void run() {
while (isOkToRun) {
sendMessage(getMessaage());
}
done.countDown();
}
public void stop() {
isOkToRun = false;
done.await();
}
}

The idea of stop() is that it didn't return until the thread had exited, so when it returned the system was in a known state. This is OK, unless sendMessage() results in the invokation of stop(), where it will wait forever. As long as stop() is never invoked from the Runnable, everything will work as you expect. In a large application, however, the activity of the Runnable's thread may not be obvious!

The solution was to call await() with a timeout of a few seconds, and to log a stack dump and complaint any time the timeout occurred. This preserved the desired behavior when it was possible, and exposed coding problems as they were encountered.

Updating a Swing UI component (typically a progress bar) in a worker thread instead of in the Swing thread (one should of course use SwingUtilities.invokeLater(Runnable), but if you forget to do this then the bug can take a long time to surface.)

public class ThreadA implements Runnable {
    private volatile SharedObject obj;

    public void run() {
        while (true) {
            obj = new SharedObject();
            obj.setValue("Hallo");
        }
    }

    public SharedObject getObj() {
        return obj;
    }
}

The problem I'm trying to point out here (among others) is that the flush of the SharedObject obj happens before setting the value "Hallo". That means that the consumer of getObj() might retrieve an instance where getValue() returns null.

public class ThreadB implements Runnable {
    ThreadA a = null;

    public ThreadB(ThreadA a) {
        this.a = a;
    }

    public void run() {
        while (true) {
            try {
                System.out.println("SharedObject: " + a.getObj().getVal());
                Thread.sleep(50);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
    }
}

public class SharedObject {
    private String val = null;

    public SharedObject() {
    }

    public String getVal() {
        return val;
    }

    public void setVal(String val) {
        this.val = val;
    }
}

Assisting with the Implementation of Actors in Functional Java and benchmarking millions of threads on multi-core machines.

while(true)
{
   if (...)
     break

   doStuff()
}

Invariably when developers write while loops they miss the "resource commit" in their own code.

Namely if that block does not exit, the application and maybe even the system will lock up and die. Just because of a simple while(fantasy_land)...if(...) break.

A nasty gotcha I've found in java is having multiple threads access a HashMap without synchronization. If one is reading and one is writing then there is a good chance of the reader ending up in an infinite loop (the bucket node list structure gets corrupted into a looped list).

Obviously you shouldn't be doing this in the first place (use ConcurrentHashMap or Collections.synch... wrapper), but it seems to be the one that always gets through the net and causes proper thread stuck, system completely broken, usually due to a utility class containing such a map being a few levels down the stack and nobody thinking of it.

I think the most frequent concurrency problem in Java is code that actually seems to work so far, although it is not really thread-safe at all. Thanks to a tiny error, it becomes a time bomb and in almost all cases, you don't know that in advance, because it is not obvious to you. While regular faulty code hopefully fails during tests, concurrent code often only fails eventually and non-reproducible.