Avoid synchronized(this) in Java?

Question

Whenever a question pops up on SO about Java synchronization, some people are very eager to point out that synchronized(this) should be avoided. Instead, they claim, a lock on a private reference is to be preferred.

Some of the given reasons are:

• some evil code may steal your lock (very popular this one, also has an "accidentally" variant)
• all synchronized methods within the same class use the exact same lock, which reduces throughput
• you are (unnecessarily) exposing too much information

Other people, including me, argue that synchronized(this) is an idiom that is used a lot (also in Java libraries), is safe and well understood. It should not be avoided because you have a bug and you don't have a clue of what is going on in your multithreaded program. In other words: if it is applicable, then use it.

I am interested in seeing some real-world examples (no foobar stuff) where avoiding a lock on this is preferable when synchronized(this) would also do the job.

Therefore: should you always avoid synchronized(this) and replace it with a lock on a private reference?

---

Some further info (updated as answers are given):

• we are talking about instance synchronization
• both implicit (synchronized methods) and explicit form of synchronized(this) are considered
• if you quote Bloch or other authorities on the subject, don't leave out the parts you don't like (e.g. Effective Java, item on Thread Safety: "Typically it is the lock on the instance itself, but there are exceptions.")
• if you need granularity in your locking other than synchronized(this) provides, then synchronized(this) is not applicable so that's not the issue

Answer 1

Well, firstly it should be pointed out that:

public void blah() {
  synchronized (this) {
    // do stuff
  }
}

is semantically equivalent to:

public synchronized void blah() {
  // do stuff
}

which is one reason not to use synchronized(this). You might argue that you can do stuff around the synchronized(this) block. The usual reason is to try and avoid having to do the synchronized check at all, which leads to all sorts of concurrency problems, specifically the double checked-locking problem, which just goes to show how difficult it can be to make a relatively simple check threadsafe.

A private lock is a defensive mechanism, which is never a bad idea.

Also, as you alluded to, private locks can control granularity. One set of operations on an object might be totally unrelated to another but synchronized(this) will mutually exclude access to all of them.

synchronized(this) just really doesn't give you anything.

Answer 2

I think points one (somebody else using your lock) and two (all methods using the same lock needlessly) can happen in any fairly large application. Especially when there's no good communication between developers.

It's not cast in stone, it's mostly an issue of good practice and preventing errors.

Answer 3

No, you shouldn't always. However, I tend to avoid it when there are multiple concerns on a particular object that only need to be threadsafe in respect to themselves. For example, you might have a mutable data object that has "label" and "parent" fields; these need to be threadsafe, but changing one need not block the other from being written/read. (In practice I would avoid this by declaring the fields volatile and/or using java.util.concurrent's AtomicFoo wrappers).

Synchronization in general is a bit clumsy, as it slaps a big lock down rather than thinking exactly how threads might be allowed to work around each other. Using synchronized(this) is even clumsier and anti-social, as it's saying "no-one may change anything on this class while I hold the lock". How often do you actually need to do that?

I would much rather have more granular locks; even if you do want to stop everything from changing (perhaps you're serialising the object), you can just acquire all of the locks to achieve the same thing, plus it's more explicit that way. When you use synchronized(this), it's not clear exactly why you're synchronizing, or what the side effects might be. If you use synchronized(labelMonitor), or even better labelLock.getWriteLock().lock(), it's clear what you are doing and what the effects of your critical section are limited to.

Answer 4

Short answer: You have to understand the difference and make choice depending on the code.

Long answer: In general I would rather try to avoid synchronize(this) to reduce contention but private locks add complexity you have to be aware of. So use the right synchronization for the right job. If you are not so experienced with multi-threaded programming I would rather stick to instance locking and read up on this topic. (That said: just using synchronize(this) does not automatically make your class fully thread-safe.) This is a not an easy topic but once you get used to it, the answer whether to use synchronize(this) or not comes naturally.

Answer 5

I'll cover each point separately.

1. Some evil code may steal your lock (very popular this one, also has an "accidentally" variant)

   I'm more worried about accidentally. What it amounts to is that this use of this is part of your class' exposed interface, and should be documented. Sometimes the ability of other code to use your lock is desired. This is true of things like Collections.synchronizedMap (see the javadoc).

2. All synchronized methods within the same class use the exact same lock, which reduces throughput

   This is overly simplistic thinking; just getting rid of synchronized(this) won't solve the problem. Proper synchronization for throughput will take more thought.

3. You are (unnecessarily) exposing too much information

   This is a variant of #1. Use of synchronized(this) is part of your interface. If you don't want/need this exposed, don't do it.

Answer 6

It depends on the task you want to do, but I wouldn't use it. Also, check if the thread-save-ness you want to accompish couldn't be done by synchronize(this) in the first place? There are also some nice locks in the API that might help you :)

Answer 7

While you are using synchronized(this) you are using the class instance as a lock itself. This means that while lock is acquired by thread 1 the thread 2 should wait

Suppose the following code

public void method1() {
    do something ...
    synchronized(this) {
        a ++;      
    }
    ................
}


public void method2() {
    do something ...
    synchronized(this) {
        b ++;      
    }
    ................
}

Method 1 modifying the variable a and method 2 modifying the variable b, the concurrent modification of the same variable by two threads should be avoided and it is. BUT while thread1 modifying a and thread2 modifying b it can be performed without any raise condition.

Unfortunately the above code will not allow this since the we are using the same reference for a lock; This means that threads even if they are not in a race condition should wait and obviously the code sacrifices concurrency of the program.

The solution is to use 2 different locks for two different variables.

  class Test {
        private Object lockA = new Object();
        private Object lockB = new Object();

public void method1() {
    do something ...
    synchronized(lockA) {
        a ++;      
    }
    ................
}


public void method2() {
    do something ...
    synchronized(lockB) {
        b ++;      
    }
    ................
 }

The above example uses more fine grained locks (2 locks instead one (lockA and lockB for variables a and b respectively) and as result allows better concurrency, on the other hand it became more complex than the first example ...

Answer 8

While I agree about not adhering blindly to dogmatic rules, does the "lock stealing" scenario seem so eccentric to you? A thread could indeed acquire the lock on your object "externally"(synchronized(theObject) {...}), blocking other threads waiting on synchronized instance methods.

If you don't believe in malicious code, consider that this code could come from third parties (for instance if you develop some sort of application server).

The "accidental" version seems less likely, but as they say, "make something idiot-proof and someone will invent a better idiot".

So I agree with the it-depends-on-what-the-class-does school of thought.

---

Edit following eljenso's first 3 comments:

I've never experienced the lock stealing problem but here is an imaginary scenario:

Let's say your system is a servlet container, and the object we're considering is the ServletContext implementation. Its getAttribute method must be thread-safe, as context attributes are shared data; so you declare it as synchronized. Let's also imagine that you provide a public hosting service based on your container implementation.

I'm your customer and deploy my "good" servlet on your site. It happens that my code contains a call to getAttribute.

A hacker, disguised as another customer, deploys his malicious servlet on your site. It contains the following code in the init method:

synchronized (this.getServletConfig().getServletContext()) {
   while (true) {}
}

Assuming we share the same servlet context (allowed by the spec as long as the two servlets are on the same virtual host), my call on getAttribute is locked forever. The hacker has achieved a DoS on my servlet.

This attack is not possible if getAttribute is synchronized on a private lock, because 3rd-party code cannot acquire this lock.

I admit that the example is contrived and an oversimplistic view of how a servlet container works, but IMHO it proves the point.

So I would make my design choice based on security consideration: will I have complete control over the code that has access to the instances? What would be the consequence of a thread's holding a lock on an instance indefinitely?

Answer 9

There seems a different consensus in the C# and Java camps on this. The majority of Java code I have seen uses:

// apply mutex to this instance
synchronized(this) {
    // do work here
}

whereas the majority of C# code opts for the arguably safer:

// instance level lock object
private readonly object _syncObj = new object();

...

// apply mutex to private instance level field (a System.Object usually)
lock(_syncObj)
{
    // do work here
}

The C# idiom is certainly safer. As mentioned previously, no malicious / accidental access to the lock can be made from outside the instance. Java code has this risk too, but it seems that the Java community has gravitated over time to the slightly less safe, but slightly more terse version.

That's not meant as a dig against Java, just a reflection of my experience working on both languages.

Answer 10

The java.util.concurrent package has vastly reduced the complexity of my thread safe code. I only have anecdotal evidence to go on, but most work I have seen with synchronized(x) appears to be re-implementing a Lock, Semaphore, or Latch, but using the lower-level monitors.

With this in mind, synchronizing using any of these mechanisms is analogous to synchronizing on an internal object, rather than leaking a lock. This is beneficial in that you have absolute certainty that you control the entry into the monitor by two or more threads.

Answer 11

If you've decided that:

• the thing you need to do is lock on the current object; and
• you want to lock it with granularity smaller than a whole method;

then I don't see the a taboo over synchronizezd(this).

Some people deliberately use synchronized(this) (instead of marking the method synchronized) inside the whole contents of a method because they think it's "clearer to the reader" which object is actually being synchronized on. So long as people are making an informed choice (e.g. understand that by doing so they're actually inserting extra bytecodes into the method and this could have a knock-on effect on potential optimisations), I don't particularly see a problem with this. You should always document the concurrent behaviour of your program, so I don't see the "'synchronized' publishes the behaviour" argument as being so compelling.

As to the question of which object's lock you should use, I think there's nothing wrong with synchronizing on the current object if this would be expected by the logic of what you're doing and how your class would typically be used. For example, with a collection, the object that you would logically expect to lock is generally the collection itself.

Answer 12

A good example for use synchronized(this).

// add listener
public final synchronized void addListener(IListener l) {listeners.add(l);}
// remove listener
public final synchronized void removeListener(IListener l) {listeners.remove(l);}
// routine that raise events
public void run() {
   // some code here...
   Set ls;
   synchronized(this) {
      ls = listeners.clone();
   }
   for (IListener l : ls) { l.processEvent(event); }
   // some code here...
}

As you can see here, we use synchronize on this to easy cooperate of lengthly (possibly infinite loop of run method) with some synchronized methods there.

Of course it can be very easily rewritten with using synchronized on private field. But sometimes, when we already have some design with synchronized methods (i.e. legacy class, we derive from, synchronized(this) can be the only solution).