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

Just that it will almost certainly slow things down.

Every sync'ed object must perform a check of some sort to ensure it's not already in use and then set flags when being used and reset them when finished.

This takes time. You probably won't notice a functional change but, if performance is important, you need to watch out.

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.

Answer 3

The performance point has already been stated.

Also, remember that all threads get a different copy of the stack. Therefore, if a method only uses variables that are created inside that method, and there's no access to the external world (e.g. file handles, sockets, DB connections, etc.), then there is no possibility of threading issues occurring.

Answer 4

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 5

If you think that putting the "synchronized" keyword everywhere is a good solution, even ignoring performance, then you don't understand exactly what is happening and you should not be using it. I highly suggest reading up on the topic before diving into it without direction.

Threading is an incredibly difficult topic to master and it's really important to understand why you are doing things. Otherwise you will up with a lot of code that works by accident rather than design. This will just end up causing you pain.

Answer 6

Far better than putting synchronized everywhere is to carefully reason about that invariants your classes need, then synchronize just enough to ensure those invariants. If you over-synchronize, you create two risks:

1. deadlock
2. liveness problems

Deadlock everyone knows. Liveness problems aren't related to the cost of synchronization, but instead to the fact that if you globally synchronize everything in a multithreaded application, then many threads will be blocked waiting to get a monitor because another thread is touching something unrelated but using the same monitor.

If you want to slap a keyword everywhere for safety, then I recommend use of final instead of synchronized. :) Anything that you can make final contributes to better thread safety and easier reasoning about what invariants need to be maintained by locks. To give an example, let's say you have this trivial class:

public class SimpleClass {
    private volatile int min, max;
    private volatile Date startTime;
    // Assume there are many other class members

    public SimpleClass(final int min, final int max) {
        this.min = min;
        this.max = max;
    }
    public void setMin(final int min) {
        // set min and verify that min <= max
    }
    public void setMax(final int max) {
        // set max and verify that min <= max
    }
    public Date getStartTime() {
        return startTime;
    }
}

With the above class, when setting min or max, you need to synchronize. The code above is broken. What invariant does this class have? You need to ensure that min <= max at all times, even when multiple threads are calling setMin or setMax.

Assuming this is a big class with many variables and you synchronize everything, then if one thread calls setMin and another thread calls getStartTime, the second thread will be unnecessarily blocked until setMin returns. If you do this with a class with many members, and assuming only a few of those members are involved in invariants that must be defended, then synchronizing everything will cause many liveness problems of this sort.

Answer 7

Let's say you have this example

synchronized (lock){
    doSomething();
}

If you have several threads locked you cannot interrupt them. It is better to use Lock objects instead of "synchronized" because you have a better control of the interrupts which can occur. Of course like all suggested there are also some performance issues with "synchronized" and if you overuse them you can have deadlocks, livelocks, etc.

Answer 8

You shouldn't, and you probably need to reconsider your design. From my experience synchronization at this level is just not scalable. It might fix your immediate problems, but won't help to fix your overall application integrity.

For example: Even if you do have a "synchronized" collection, threads may want to call two "synchronized" methods, but don't want another thread to see the intermediate result. So any caller of a fine grained "synchronized" API must be itself aware of that, which heavily degrades the (programmer-) usability of this API.

For starters it is best to use worker threads. Isolate specific long running tasks so that separate threads take one block of immutable input and - when finished - enqueue the result in the main thread's message queue (this of course should be synchronized). The main thread may poll or explicily wait for a signal if it has nothing else to do. Details depend on your performance requirements and application design.

On the long run, try to understand implementations of the ACID transaction model of database implementations or message passing concurrency, ... the Erlang language will give you a lot of inspiration. These are synchronization models that are proven scalable. Take the best of them and try to port these concepts to the multithreaded application you work at.

Answer 9

If performance isn't too much of an issue, why don't you use one thread and then you can safely remove all "synchronized" If you synchronize everything you have effectively achieved the same thing as only one thread can do anything , but in a much more complicated and error prone way.

Ask yourself, why are you using more than one thread?