Synchronization with Modification of the Lock Reference

2014-03-25 by Tomas Tulka

...is very bad practice. Nevertheless, it is not so rare to meet it.

In legacy sources I have found a really tricky code causing an occasional error.

Well, you can say all around synchronization is tricky, but good understanding is a clue to eliminate the magic - this article might help a bit.

Let's consider a very basic logic of the code:

Thread-agents are dealing with a shared data of the singleton system.

Alright, easy, the code working with the shared data must be synchronized. Yes, but don't forget that the threads (the agents) are working with data of another class (the system) - we can not use synchronized methods as they are using the instance of an agent as a mutex instead of the class the data belongs to. Well, let's use the shared system's object as a lock and... we are ready... wait a minute: This is exactly the solution implemented in the legacy code!

Where's the problem - this must work! Sure, but not in any case...

In a very simplified way the original code looks like this:

public class SystemApp {

    static Integer count = 0;

    static class AgentThread extends Thread {

        public void run() {
            try {
                Thread.sleep(ThreadLocalRandom.current().nextInt(100));
            } catch (InterruptedException e) { }   

            // do something
            // ...

            synchronized (count) {
                count ++;
            }

            // do something else
            // ...
        }
    }

    public static void main(String[] argv) {       
        // run agents
        for (int i = 0; i < 1000; i ++) {
            new AgentThread().start();
        }

        // sleep for a while to let agents finish their work
        try {
            Thread.sleep(5000);
        } catch (InterruptedException e) { }   

        // print the result
        System.out.println(count);
    }
}

What is the expected result printed on the screen? 1000? Right!

But guess what we get by running it five times:

955
979
973
959
963

Of course, the pain is in here:

synchronized (count) {
    count ++;
}

We are using as a lock an object referenced by the variable count. But this reference is changed inside the critical section. This means there is no guarantee that two or more parallel threads get the same lock object. And so sometimes happens this scenario:

  1. The thread A and the thread B are trying to enter the critical section.
  2. The thread A gets the access and the lock referenced by the variable count (currently for instance with a value of integer=2) gets locked up.
  3. The thread B is waiting for the release of the lock (integer=2).
  4. The thread A changes the reference of the variable count to the new value (integer=3) and releases the lock (integer=2).
  5. The thread B waiting for the lock (integer=2) gets that lock, enters the critical section and locks up the lock object (integer=2).
  6. The parallel thread C tries to enter the critical section with the lock object referenced by the variable count (integer=3 - changed by the thread A).
  7. The tread C gets the access to the critical section because its lock is now the object integer=3 while the thread B is being in the critical section as well (with the lock object integer=2).

As you can see, by the seventh point we have two threads in the critical section.

The lesson is: never ever use a variable object as a lock!


Synchronization is tricky and it is always good to think twice.

By the way, using the API from the java.util.concurrent package is a good idea, too.