Any reason NOT to slap the 'synchronized' keyword everywhere?

Question

In my java project, almost every non-static method I've written is synchronized. I've decided to fix up some code today, by removing most of the synchronized keywords. Right there I created several threading issues that took quite a while to fix, with no increase in performance. In the end I reverted everything.

I don't see anyone else writing code with "synchronized" everywhere. So is there any reason I shouldn't have "synchronized" everywhere?

What if I don't care too much about performance (ie. the method isn't called more than once every few seconds)?

Answer 1

If you indiscriminately synchronize, you also run the risk of creating a deadlock.

Suppose I have two classes, Foo and Bar, both having a synchronized method doSomething(). Suppose further that each class has a synchronized method taking an instance of the other class as a parameter.

public class Foo {

    synchronized void doSomething() {
        //code to doSomething
    }

    synchronized void doSomethingWithBar(Bar b) {
        b.doSomething();
    }

}

public class Bar {

    synchronized void doSomething() {
        //code to doSomething
    }

    synchronized void doSomethingWithFoo(Foo f) {
        f.doSomething();
    }
}

You can see that if you have an instance of Foo and an instance of Bar, both trying to execute their doSomethingWith* methods on each other at the same time, a deadlock can occur.

To force the deadlock, you could insert a sleep in both the doSomethingWith* methods (using Foo as the example):

synchronized void doSomethingWithBar(Bar b) {
    try {
        Thread.sleep(10000);
    } catch (InterruptedException ie) {
        ie.printStackTrace();
    }

    b.doSomething();
}

In your main method, you start two threads to complete the example:

public static void main(String[] args) {
    final Foo f = new Foo();
    final Bar b = new Bar();
    new Thread(new Runnable() {
        public void run() {
            f.doSomethingWithBar(b);
        }
    }).start();

    new Thread(new Runnable() {
        public void run() {
            b.doSomethingWithFoo(f);
        }
    }).start();
}

Answer 2

Of course - performance. Monitors have a cost.

The answer is neither removing nor adding synchronization in a random fashion. Better to read something like Brian Goetz' "Java Concurrency In Practice" or Doug Lea's "Java Threads" books to understand how to do it properly. And learn the new concurrent packages well, of course.

Multithreading is a lot more than the synchronized keyword.