Avoid Synchronized(This) in Java

Avoid synchronized(this) in Java?

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.

Avoid synchronized at method level

Yes, it is technically the same. This is about code style, not correctness.

The link you posted explains the reason quite well:

Method-level synchronization can cause problems when new code is added
to the method. Block-level synchronization helps to ensure that only
the code that needs synchronization gets it.

Does synchronized (this) lock only the synchronized block or all the this code?

Using synchronized means in order for a thread to execute that block or method, it has to acquire a lock referenced (explicitly or implicitly) by that block or method. For the static synchronized methods, that lock is the monitor on the class object. For the synchronized(this) block, the lock used is the monitor on the current instance. Sharing of locks between multiple methods or blocks is what enforces atomicity and memory visibility of updates, also the shared lock provides a shared communication path through which waiting and notification can take place.

Since the static synchronized blocks use a different lock from that used by the block in the constructor, entering a static synchronized block is not blocked by another thread's accessing the block that requires acquiring the lock on the current instance, and the synchronized block in the constructor has no effect on anything, the lock acquisition will always be uncontended. More importantly here, changes made by one thread in the constructor may not get seen by other threads using the getter. Synchronization affects both locking and memory visibility.

This changed version would work:

public class ObjectCounter {
    private static long numOfInstances = 0;
    public ObjectCounter(){
        synchronized(ObjectCounter.class){
            numOfInstances++;
        }
    }
    public static synchronized long getCount(){
        return numOfInstances;
    }
}

because the getter and the incrementing block are using the same lock. Making the different threads acquire the same monitor ensures that the change to the counter gets safely published so that another thread accessing the getter can see the updated value.

The synchronized keyword says, "you have to acquire a lock before you can enter", where for the method the lock is assumed: with the static keyword on the method it's the monitor on the class, without a static keyword it's the monitor on the current instance. For locking to work correctly the different blocks and methods need to use the same lock. There is arguably too much syntax sugar and too much making things convenient in how Java was designed: allowing implicit choice of locks and putting the monitor on java.lang.Object can cause confusion.

WRT your question #6: For what you're doing here you'd be better off with an AtomicLong. Use synchronized blocks for coordinating multiple changes that need to take place without interference from other threads.

Questions #3, #7 and #8 seem very similar: If a method/block isn't attempting to acquire a lock, nothing prevents threads from executing that method/block. The object as a whole doesn't get any protection, using the synchronized methods or blocks to enforce locking is what does the protecting. Think less in terms of "using the synchronized keyword" and more in terms of what lock threads need to acquire.

How stale data is avoided using synchronized keyword?

The issue you fix this way is not that thread B executes the set immediately after A executes a get, that one will still return the "old" (well, technically correct at the time, but soon to be wrong) value.

The issue the synchronization fixes is that even if thread B wrote before thread A read, A could read an old value due to caching (most likely CPU caches, but this depends on the JVM implementation). A non-synchronized read from a non-volatile variable can use a cached value. In other words: the synchronized creates a read-barrier, which means "you have to re-read this value, even if you already have it in your CPU cache".

Note that for this specific case, simply adding volatile to value would have the same effect, but for more complex access patterns synchronized (or it's equivalence in newer APIs Lock) is necessary.

Synchronized methods to avoid deadlock

for example when a thread has a lock on a variable res1 but needs a lock on variable res2

What matters is not that there are two variables, what matters is that there must be two (or more) locks.

The names "res1" and "res2" are meant to suggest two resources each of which may have one or more variables, and each of which has its own lock. Here's where you get into trouble:

final Object lock1 = new Object();
final Object lock2 = new Object();

public void method1() {
    synchronized (lock1) {
        // Call Thread.sleep(1000) here to simulate the thread losing its time slice.
        synchronized(lock2) {
            doSomethingThatRequiresBothLocks
        }
    }
}

public void method2() {
    synchronized (lock2) {
        // Do the same here 'cause you can't know which thread will get to run first.
        synchronized(lock1) {
            doSomethingElseThatRequiresBothLocks()
        }
    }
}

If thread A calls method1(), there is a very small chance that it could lose its time slice (i.e., turn to run) just after it successfully locks lock1, but before it locks lock2.

Then, while thread A is waiting its turn to run again, thread B calls method2(). Thread B will be able to lock lock2, but then it gets stuck because lock1 is locked by thread A. Furthermore, when thread A gets to run again, it will immediately be blocked when it tries to lock lock2 which is owned by thread B. Neither thread will ever be able to continue from that point.

---

In real code, it's never so obvious. When it happens in real-life, it usually is because of some unforseen interaction between code from two or more different modules that may not even be aware of each other, but which access the same common resources.

Questions about how the synchronized keyword works with locks and thread starvation

First you can not pass primitive variable to synchronized, it requires a reference. Second that tutorial is just a example showing guarded block. It's not c1,c2 that it's trying to protect but it's trying to protect all the code inside synchronized block.

JVM uses Operating system's scheduling algorithm.

What is the JVM Scheduling algorithm?

So it's not JVM's responsibility to see if threads are starved. You can however assign priority of threads to prefer one over other to execute.

Every thread has a priority. Threads with higher priority are executed in preference to threads with lower priority. Each thread may or may not also be marked as a daemon. When code running in some thread creates a new Thread object, the new thread has its priority initially set equal to the priority of the creating thread, and is a daemon thread if and only if the creating thread is a daemon.

From:https://docs.oracle.com/javase/7/docs/api/java/lang/Thread.html

If you're concerned about this scenario then you have to implement it yourself. Like maintaining a thread which checks for starving threads and as time passes it increases the priority of the threads which have been waiting longer than others.

Yes it's true that two method which have been synchronized will never be executed on the same instance simultaneously.

Java synchronized method lock on object, or method?

If you declare the method as synchronized (as you're doing by typing public synchronized void addA()) you synchronize on the whole object, so two thread accessing a different variable from this same object would block each other anyway.

If you want to synchronize only on one variable at a time, so two threads won't block each other while accessing different variables, you have synchronize on them separately in synchronized () blocks. If a and b were object references you would use:

public void addA() {
    synchronized( a ) {
        a++;
    }
}

public void addB() {
    synchronized( b ) {
        b++;
    }
}

But since they're primitives you can't do this.

I would suggest you to use AtomicInteger instead:

import java.util.concurrent.atomic.AtomicInteger;

class X {

    AtomicInteger a;
    AtomicInteger b;

    public void addA(){
        a.incrementAndGet();
    }

    public void addB(){ 
        b.incrementAndGet();
    }
}

Does using synchronized makes this code sequential?

Yes. The synchronized keyword on non-static methods restricts the given method to sequential execution per instance. You only have one instance of Counter and are reusing it for all tasks, so, even though you have a thread pool with 2 threads, only one will be executing run() at any given time.

---

Related Topics