Why is this class not thread safe?

class ThreadSafeClass extends Thread
{
     private static int count = 0;

     public synchronized static void increment()
     {
         count++;
     }

     public synchronized void decrement()
     {
         count--;
     }
}

Can anyone explain why above class is not thread safe?

Since the increment method is static it will synchronize on the class object for the ThreadSafeClass. The decrement method is not static and will synchronize on the instance used to call it. I.e., they will synchronize on different objects and thus two different threads can execute the methods at the same time. Since the ++ and -- operations are not atomic the class is not thread safe.

Also, since count is static, modifying it from decrement which is a synchronized instance method is unsafe since it can be called on different instances and modify count concurrently that way.

You have two synchronized methods, but one of them is static and the other is not. When accessing a synchronized method, based on it's type (static or non-static), a different object will be locked. For a static method, a lock will be put on the Class object, while for the non-static block, a lock will be put on the instance of the class that runs the method. Because you have two different locked objects, you can have two threads that modify the same object simultaneously.

> Can anyone explain why above class is not thread safe?

• increment being static, synchronization will be done on the class itself.
• decrement being not static, synchronization will be done on the object instantiation, but that doesn't secure anything as count is static.

I'd like to add that to declare a thread-safe counter, I believe the simplest way is to use AtomicInteger instead of a primitive int.

Let me redirect you to the java.util.concurrent.atomic package-info.

Others' answers are pretty good explained the reason. I just add something to summarize synchronized:

public class A {
    public synchronized void fun1() {}

    public synchronized void fun2() {}

    public void fun3() {}

    public static synchronized void fun4() {}

    public static void fun5() {}
}

A a1 = new A();

synchronized on fun1 and fun2 is synchronized on instance object level. synchronized on fun4 is synchronized on class object level. Which means:

1. When 2 threads call a1.fun1() at same time, latter call will be blocked.
2. When thread 1 call a1.fun1() and thread 2 call a1.fun2() at same time, latter call will be blocked.
3. When thread 1 call a1.fun1() and thread 2 call a1.fun3() at same time, no blocking, the 2 methods will be executed at same time.
4. When thread 1 call A.fun4(), if other threads call A.fun4() or A.fun5() at same time, latter calls will be blocked since synchronized on fun4 is class level.
5. When thread 1 call A.fun4(), thread 2 call a1.fun1() at same time, no blocking, the 2 methods will be executed at same time.

1. decrement is locking on a different thing to increment so they do not prevent each other from running.
2. Calling decrement on one instance is locking on a different thing to calling decrement on another instance, but they are affecting the same thing.

The first means that overlapping calls to increment and decrement could result in a cancel-out (correct), an increment or a decrement.

The second means that two overlapping calls to decrement on different instances could result in a double decrement (correct) or a single decrement.

Since two different methods, one is instance level and other is class level, so you need to lock on 2 different objects to make it ThreadSafe

As explained in other answers, your code is not Thread safe since static method increment() locks Class monitor and non-static method decrement() locks Object monitor.

For this code example, better solution exists without synchronzed keyword usage. You have to use AtomicInteger to achieve Thread safety.

Thread safe using AtomicInteger:

import java.util.concurrent.atomic.AtomicInteger;

class ThreadSafeClass extends Thread {

    private static AtomicInteger count = new AtomicInteger(0);

    public static void increment() {
        count.incrementAndGet();
    }

    public static void decrement() {
        count.decrementAndGet();
    }

    public static int value() {
        return count.get();
    }

}