Why is this class not thread safe?

Question

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?

I don't know about Java, but it looks like each of those methods are *individually* thread-safe, but you could have a thread in **each** of the methods at the same time. Maybe if you have a single method which takes a bool (`increment`) it'd be thread safe. Or if you used some locking object. As I said, I don't know about Java - my comment stems from C# knowledge. — Wai Ha Lee, May 13 '15 at 07:41
See http://stackoverflow.com/questions/7805192/is-a-volatile-int-in-java-thread-safe — Tagir Valeev, May 13 '15 at 07:42
I also don't know Javavery well, but to synchronize access to a static variable, the `synchronized` should be used only in static methods. So in my opionion even if you removes the `increment` method, it's still not threadsafe since two instances (which have only synchronized access through the same instance) can call the method concurrently. — Onur, May 13 '15 at 14:36
It is thread safe as long as you never create an instance of the class. — Benjamin Gruenbaum, May 13 '15 at 20:37
This question seems perfectly clear to me, though probably not deserving of as many votes as it received (maybe it was in the Hot Network Questions). (Presumably the asker thought it's not thread-safe because someone (or an IDE/tool) told him it isn't, but not why.) — Jeffrey Bosboom, May 17 '15 at 01:38

K Erlandsson · Accepted Answer · 2016-06-30T07:49:17.880

134

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.

edited Jun 30 '16 at 07:49

answered May 13 '15 at 07:45

K Erlandsson

13,408
6
51
67

12

You may add, since `count` is `static`, having an instance method `decrement()` is wrong, even if there was no `static` `increment()` method, as two threads can invoke `decrement()` on different instances modifying the same counter concurrently. – Holger May 13 '15 at 07:49
1

That's possibly a good reason to prefer using `synchronized` blocks in general (even on the whole method content) instead of using the modifier on the method, i.e. `synchronized(this) { ... }` and `synchronized(ThreadSafeClass.class) { ... }`. – Bruno May 13 '15 at 10:13
[`++` and `--` are not atomic, even on `volatile int`](http://stackoverflow.com/a/7805212/1986871). `Synchronized` takes care of the read/update/write problem with `++`/`--`, but the `static` keyword is, well, *key* here. Good answer! – Chris Cirefice May 13 '15 at 15:49
Re, _modifying [a static field] from...a synchronized instance method is wrong_: There is nothing inherently wrong with accessing a static variable from within an instance method, and there is nothing inherently wrong with accessing it from a `synchronized` instance method either. Just don't expect "synchronized" on the instance method to provide protection for the static data. The only problem here is what you said in your first paragraph: The methods use different locks in an attempt to protect the same data, and that of course provides no protection at all. – Solomon Slow May 13 '15 at 16:11

score 23 · Answer 2 · answered May 13 '15 at 07:47

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.

score 14 · Answer 3 · answered May 13 '15 at 12:01

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.

score 8 · Answer 4 · answered May 17 '15 at 05:37

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:

When 2 threads call a1.fun1() at same time, latter call will be blocked.
When thread 1 call a1.fun1() and thread 2 call a1.fun2() at same time, latter call will be blocked.
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.
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.
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.

Jon Hanna · Answer 5 · 2015-05-13T10:50:39.443

decrement is locking on a different thing to increment so they do not prevent each other from running.
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.

score 4 · Answer 6 · answered May 13 '15 at 10:18

4

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

answered May 13 '15 at 10:18

jaleel_quest

41
2

score 1 · Answer 7 · answered May 20 '16 at 18:06

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();
    }

}

Why is this class not thread safe?

7 Answers7

Linked