views:

9023

answers:

48

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. Thanks!

+26  A: 

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.

Alex Miller
There is an IDEA inspection for this called "Synchronization on non-final field unlikely to have useful semantics". Very nice.
Hamlet D'Arcy
Ha...now that's a tortured description. "unlikely to have useful semantics" could better be described as "most likely broken". :)
Alex Miller
I think it was Bitter Java that had this in its ReadWriteLock. Fortunately we now have java.util.concurrency.locks, and Doug is a bit more on the ball.
Tom Hawtin - tackline
I also have seen this problem often. Only sync on final objects, for that matter. FindBugs et al. help, yes.
gimpf
is this only a problem during assignment? (see @Alex Miller's example below with a Map) Would that map example have this same problem as well?
Alex Beardsley
@Nalandial - not sure what you're asking. It's not a problem to mutate the state of the object under assignment; that's very common. It's a problem to change the assignment so that different threads might be synchronizing on different objects though, which is what I was getting at here.
Alex Miller
Do you know of any open source project containing this bug in some version of it? I'm looking for concrete examples of this bug in real world software.
reprogrammer
@reprogrammer - Nope. If I did, I would tell them to fix it. :)
Alex Miller
+2  A: 

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

This leads to very bad performance because of contention.

kohlerm
Well, sometimes, sometimes not. If it would just be that easy...
gimpf
Assuming that threading helps at all to increase performance for the problem given, it does always degrade performance as soon as more than one thread accesses code that is protected by the lock.
kohlerm
+7  A: 

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.

Dave Ray
Can you split the last one out into a separate answer? Let's keep it 1 per post. These are two really good ones.
Alex Miller
Sure, no problem.
Dave Ray
+6  A: 

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.

Alex Miller
+2  A: 

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.

Brian Clapper
Are you implying that you run into *more* problems now that java.util.concurrent is available?
Andrzej Doyle
+5  A: 

The dumbest mistake I frequently make is forgetting to synchronize before calling notify() or wait() on an object.

Dave Ray
Unlike most concurrency problems, isn't this one easy to find? At least you get an IllegalMonitorStateException here...
Outlaw Programmer
Thankfully it is very easy to find ... but it's still a dumb mistake that wastes my time more than it should :)
Dave Ray
+21  A: 

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); }
Fabian Steeg
No, that's totally what I'm looking for. Thanks!
Alex Miller
+6  A: 

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.

bcash
+15  A: 

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. It should be:

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

Or with a ConcurrentMap implementation:

map.putIfAbsent("foo", "bar");
Dave Ray
Or better, use a ConcurrentHashMap and putIfAbsent.
Tom Hawtin - tackline
+33  A: 

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.

Alex Miller
Do you know of any open source project containing this bug in some version of it? I'm looking for concrete examples of this bug in real world software.
reprogrammer
+14  A: 

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
 }
Alex Miller
+2  A: 

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.

Hamlet D'Arcy
A: 

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

Javamann
While this is certainly a problem, it is not a concurrency bug as requested in the original question.
Alex Miller
+7  A: 

Thinking you are writing single-threaded code, but using mutable statics (including singletons). Obviously they will be shared between threads. This happens surprisingly often.

Tom Hawtin - tackline
Yes, indeed! Mutable statics break thread confinement. Surprisingly, I never found anything about this pitfall in either JCiP or CPJ.
Julien Chastang
+3  A: 

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

Tom Hawtin - tackline
+2  A: 

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.

Tom Hawtin - tackline
+14  A: 

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.

Eric Burke
one of those answers I wish I could give more than one point for
Epaga
+15  A: 

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.

Eric Burke
Yep, and there are good tools to handle this now with handlers.
Alex Miller
+24  A: 

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.

Kirk Wylie
The enum singleton pattern solves all of these problems (see Josh Bloch's comments on this). The knowledge of its existence should be more widespread among Java programmers.
Robin
I still have yet to run across a single case where lazy initialization of a singleton was actually appropriate. And if it is, just declare the method synchronized.
Dov Wasserman
This is what I use for Lazy initialization of Singleton classes. Also no synchronization required as this is guaranteed by java implicitly.class Foo { static class Holder { static Foo foo = new Foo(); } static Foo getInstance() { return Holder.foo; }}
Irfan Zulfiqar
Irfan, that's called Pugh's method, from what I recall
Chris R
@Robin, isn't it simpler to just use a static initializer? Those are always guaranteed to run synchronized.
matt b
@Matt, not if you want/need lazy initialization. However, in a DI context, usually you would just let your IoC container solve the problem for you. Static initializers don't support lazy initialization (where you don't know if you want to spend the cost in the first place), or late initialization (where you can't guarantee the ordering of a number of interdependent initialization).
Kirk Wylie
+2  A: 

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.

Eric Burke
This one bit me this week. I had destroy() but did not do a good job defining the init() side resulting in some races.
Alex Miller
+5  A: 

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 ...
    }
}
Ludwig Wensauer
+6  A: 

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

synchronized (new Object())
{
    System.out.println("sdfs");
}

This is useless.

Ludwig Wensauer
This is *probably* useless, but the act of synchronizing at all does some interesting things... Certainly creating a new Object every time is a complete waste.
TREE
A: 

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.

Ludwig Wensauer
+7  A: 

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.

Steve McLeod
A: 

mutable static variables and Singletons have been my biggest source of concurrency issues.

Parag
These have already been separately listed elsewhere - please vote them up instead.
Alex Miller
A: 
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;
    }
}
david
Would be killer to have a short explanation here too...there seems to be more than one issue here.
Alex Miller
+39  A: 

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

Kutzi
A great solution to this is to make the stop instance variable an AtomicBoolean. It solves all the problems of the non-volatile, while shielding you from the JMM issues.
Kirk Wylie
I use this method all the time, could you elaborate on what problems it causes? It has always seemed to work, but I have never cared if it took 1 ms or 100ms to finally see the change.
Karl
Karl, the problem with this approach is that you have absolutely no guarantee WHEN a thread checking the stop flag will see a modification to it by another thread. It may be 100ms with your current platform and VM, but it could be several minutes on another.
Kutzi
And if the thread to be stopped is doing something critical (like wasting the money of your company) every second on which the thread doesn't stop could count.
Kutzi
It's worse than 'for several minutes' -- you might NEVER see it. Under the Memory Model, the JVM is allowed to optimize while(!stop) into while(true) and then you're hosed. This may only happen on some VMs, only in server mode, only when the JVM recompiles after x iterations of the loop, etc. Ouch!
Cowan
Why would you want to use AtomicBoolean over volatile boolean? I'm developing for version 1.4+, so are there any pitfalls with just declaring volatile?
Pool
Nick, I think it's because atomic CAS is usually even faster than volatile.If you're developing for 1.4 your only safe option IMHO is to use synchronized as volatile in 1.4 hasn't got the strong memory barrier guarantees as it has in Java 5.
Kutzi
Wow. That's an amazing thing to find out. I need to go and fix some bugs now.
Chris R
I've never seen this fail in practice. Is there a way to make it fail?
Jon
Can anybody describe for the rest of us, why this doesn't work?
Thomas Ahle
@Thomas: that's because of the Java memory model. You should read about it, if you want to know it in detail (Java Concurrency in Practice by Brian Goetz explains it good e.g.).In short: unless you use memory synchronization keywords/constructs (like volatile, synchronized, AtomicXyz, but also when a Thread is finished) one Thread has NO guarantee whatsoever to see the changes made to any field done by a different thread
Kutzi
+6  A: 

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.

Scott Bale
+6  A: 

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

Tim Jansen
I find this mildly surprising, but I know you're a smart dude Tim so I'll take it w/o a reference. :)However, if a was final, this would not be a concern, correct? You would then be bound by final freeze semantics during construction?
Alex Miller
I still find things in the JMM that suprise me, so I wouldn't trust me, but I am pretty sure about this. See also http://www.cs.umd.edu/~pugh/java/memoryModel/jsr-133-faq.html#incorrectlySync . If the field was final it wouldn't be a problem, then it would be visible after the initialization phase.
Tim Jansen
This is only a problem, if the reference of the freshly created instance is in use already before the constructor has returned/finished. For instance the class registers itself during construction in a public pool and other threads start to access it.
ReneS
+6  A: 

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

Kutzi
This is not a problem, but unnecessary, because it tells the JVM sync the data against the global memory and therefore might run badly on multi-cpus even so, nobody uses the synchronization block in a concurrent fashion.
ReneS
+6  A: 

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.

Alex Miller
Potentially it's locked. The problem is that the semantics on WHEN Strings are interned is undefined (or, IMNSHO, underdefined). A compiler-time constant of "foo" is interned, "foo" coming in from a network interface is only interned if you make it so.
Kirk Wylie
Right, that's why I specifically used a literal string constant, which is guaranteed to be interned.
Alex Miller
+49  A: 

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.

Jared
This is one of the reasons why I prefer private static final Object LOCK = new Object();
Andrzej Doyle
I love it - oh, this is nasty :)
Thorbjørn Ravn Andersen
That's a good one for Java Puzzlers 2.
Dov Wasserman
ha, this is great
Epaga
It wasn't very "great" when I was troubleshooting it in production :P....but in hindsight, it's at least ironic.
Jared
Actually...it really makes me want the compiler to refuse to allow you to synchronize on a String. Given String interning, there is no case where that would be a "good thing(tm)".
Jared
+1 private static final Object LOCK = new Object();
parkr
I don't think this is exactly correct. It is not guaranteed that you will get the same String instance each time. Same value yes, same instance, maybe.
matt b
It's the "maybe" that hurts you. Until the string is interned, you get a different instance, but once it's been interned, you do indeed get exactly the same instance (by oid.) Additionally - you can't control when the String gets interned - the JVM may well do it for you as soon as it's created. This is exactly the same bug that happens when you use == instead of .equals on a String - and that's a very well known failure case.
Jared
@Jared: "until the string is interned" makes no sense. Strings don't magically "become" interned. String.intern() returns a different object, unless you already have the canonical instance of the specified String. Also, all literal strings and string-valued constant expressions are interned. Always. See the docs for String.intern() and §3.10.5 of the JLS.
Laurence Gonsalves
"However; because Strings are interned in Java" should really be "However; because String literals are interned in Java". (Not all strings are interned, just literals) Certainly "LOCK" is interned, which is the root of this problem.
djb
Do you remember the two different open source projects containing this bug? I'm looking for concrete examples of this bug in real world software.
reprogrammer
+2  A: 

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.

Ben Manes
A: 

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.

A: 

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

A: 

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.

This is a problem but I don't see it as a concurrency bug?
Alex Miller
A: 

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.
netzwerg
This is good advice to be sure, but does not list a Java concurrency bug.
Alex Miller
+1  A: 

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

JodaStephen
+7  A: 

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.

+10  A: 

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:

 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.

good example. I think I'd chalk this up more generally as "composition of atomic operations is not atomic". (See volatile field++ for another simple example)
Alex Miller
+5  A: 

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.

Eddie
+1  A: 

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.

Eddie
+7  A: 

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;  
    }
}
John Russell
In the later JVM's (1.5 and forward, I think), the use of volatile will fix that as well.
James Schek
Not necessarily. volatile gives you the latest value so it prevents the returning of 1 forever, but it does not provide locking. Its close, but not quite the same.
John Russell
+3  A: 

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.

grayger
+1  A: 

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.

Dov Wasserman
+1  A: 

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.

A: 

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

finnw
A: 

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.

DaveC