1

Is this Java class thread safe or reset method needs to be synchronized too? If yes can someone tell me the reason why?

public class NamedCounter {
   private int count;
   public synchronized void increment() { count++; }
   public synchronized int getCount() { return count; }
   public void reset() { count = 0; }
}
Duncan Jones
  • 67,400
  • 29
  • 193
  • 254
L4zy
  • 327
  • 2
  • 13
  • 2
    There's no way to answer this question without knowing the desired semantics. What guarantees are you expecting, if any? – David Schwartz Jul 04 '13 at 08:33
  • 7
    why are you not using [**AtomicInteger**](http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/atomic/AtomicInteger.html) ? – jlordo Jul 04 '13 at 08:33

5 Answers5

8

Not without synchronizing rest() and adding more methods. You will run into cases where you will need more methods. For example 

NamedCounter counter = new NamedCounter();
counter.increment();
// at this exact time (before reaching the below line) another thread might change changed the value of counter!!!!
if(counter.getCount() == 1) {
    //do something....this is not thread safe since you depeneded on a value that might have been changed by another thread
}

To fix the above you need something like 

NamedCounter counter = new NamedCounter();
if(counter.incrementAndGet()== 1) { //incrementAndGet() must be a synchronized method
    //do something....now it is thread safe
}

Instead, use Java's bulit-in class AtomicInteger which covers all cases. Or if you are trying to learn thread safety then use AtomicInteger as a standard (to learn from).

For production code, go with AtomicInteger without even thinking twice! Please note that using AtomicInteger does not automatically guarantee thread safety in your code. You MUST make use of the methods that are provided by the api. They are there for a reason.

Multithreader
  • 878
  • 6
  • 14
  • 1
    Just note that, under serious contention, CAS will perform *worse* than locking due to excessive retries. CAS is perfect for low-to-medium contention, just like any other *optimistic locking* scheme. – Marko Topolnik Jul 04 '13 at 09:06
  • Here is a read [CAS vs Locking](http://stackoverflow.com/questions/2664172/java-concurrency-cas-vs-locking) – Multithreader Jul 04 '13 at 09:15
2

Note that synchronized is not just about mutual exclusion, it is fundamentally about the proper ordering of operations in terms of the visibility of their actions. Therefore reset must be synchronized as well, otherwise the writes it makes may occur concurrently to other two methods, and have no guarantee to be visible.

To conclude, your class is not thread-safe as it stands, but will be as soon as you synchronize the reset method.

Marko Topolnik
  • 195,646
  • 29
  • 319
  • 436
  • 2
    I don't think this answer makes sense. If it's not synchronized by the caller, it's not safe whether or not it's synchronized itself. Consider one thread calling `reset` and another calling `increment` at the same time. The result is not guaranteed even with `synchronized` on `reset`. So this is neither sufficient nor necessary to give `reset` predictable behavior. – David Schwartz Jul 04 '13 at 08:35
  • @DavidSchwartz What has be synchronized highly depends on the use case. Therefore you don't know where to synchronize as long as you don't know the exact use case. – Uwe Plonus Jul 04 '13 at 08:38
  • @UwePlonus: Exactly. And since this answer's suggestion is neither necessary nor sufficient to make the class predictable, it's a bad answer. – David Schwartz Jul 04 '13 at 08:38
  • 1
    @DavidSchwartz You have not raised any kind of serious issue. `reset` may perfectly consistently be called concurrently with `increment`. – Marko Topolnik Jul 04 '13 at 08:38
  • 1
    @MarkoTopolnik: If the value is `2` before the concurrent call, what is the final result? Hint: It's unpredictable. – David Schwartz Jul 04 '13 at 08:39
  • @DavidSchwartz You won't have a concurrent call if 2 threads are calling different methods of the NamedCounter object. the call will be synchronized. – David Hofmann Jul 04 '13 at 08:40
  • @DavidSchwartz It is perfectly predictable because all the actions ared ordered: after `reset` the value is 0, and the next call will definitely observe that value. – Marko Topolnik Jul 04 '13 at 08:40
  • @DavidHofmann: Okay, so what will the final value be? One thread calls `reset` and another calls `increment` at the same time. The value was 2 before this. What is the final value? – David Schwartz Jul 04 '13 at 08:40
  • @DavidHofmann: In other words, it's unpredictable. So this "solution" leaves the behavior of `reset` unpredictable unless it's synchronized externally. But it was already predictable in that case. The caller of `reset` must ensure it is ordered with respsect to calls like `increment` if it wants predictable behavior, with or without this suggested change. – David Schwartz Jul 04 '13 at 08:41
  • David, please clarify with a longer explanaition. I really want to get what is on your mind because this is important for me. Why do you insist on the need to synchronize externally ? – David Hofmann Jul 04 '13 at 08:42
  • @MarkoTopolnik: How do you know what the OP needs? But if you assume he needs predictable results when he calls `increment`, possibly concurrently with another write operation, he *must* already be synchronizing those calls himself. Otherwise, he won't get predictable behavior even with your change. So I'm not sure what you're assuming, but it sure doesn't make sense. – David Schwartz Jul 04 '13 at 08:45
  • @DavidHofmann: Either the OP needs predictable results from `reset` or he doesn't. If he does, making the method `synchronized` isn't enough -- its ordering with respect to other modifications must be enforced too. And enforcing that ordering is sufficient without it being `synchronized` -- so why make it `synchronized`? What problem does that solve? – David Schwartz Jul 04 '13 at 08:46
  • @MarkoTopolnik: But it would behave consistently with some enforced synchronization order even without your change. You have to assume the OP needs *exactly* what you are giving him, but you have no basis for that assumption. – David Schwartz Jul 04 '13 at 08:48
  • @MarkoTopolnik: That's why I said "with some enforced synchronization order". Look, with or without your change, the effects of calling `reset` and `increment` at the same time are unpredictable. So if he needed predictable results, he'd have to already be enforcing the execution order elsewhere -- with or without your suggested change. Do we agree on that? – David Schwartz Jul 04 '13 at 08:50
  • @DavidSchwartz So you do agree that his class is *not thread-safe* as given, and becomes thread-safe with my proposed change. This is the basic assumption behind OP's question. – Marko Topolnik Jul 04 '13 at 08:51
  • @MarkoTopolnik: No. I don't. Not unless you make tons of assumptions about how he is calling the class or how he intends to use it. Its behaviour is unpredictable with or without your change unless the caller ensures the calls to `increment` and calls to `reset` have their order enforced elsewhere in the code. (And many classes are considered thread safe even though you cannot modify them and read them concurrently in different threads.) – David Schwartz Jul 04 '13 at 08:52
  • @DavidSchwartz When *attempting* to call `reset` and `increment` at the same time, one method will be called first then the next one, and the entire behavior will be consistent with this ordering. You seem to insist there is a need for higher-level coordination, which may or may not be true. – Marko Topolnik Jul 04 '13 at 08:53
  • @DavidSchwartz Apparently you have your own definiton of "thread-safe" under which `AtomicInteger` is not thread-safe. – Marko Topolnik Jul 04 '13 at 08:55
  • @DavidSchwartz A regular `int` is not thread-safe, you are correct. The actions on it are not properly synchronized, therefore have no ordering between them. Any thread may observe any value that has been written at some point. One thread may get a value from one second ago, another from 10 minutes ago. That is definitely not what we call "thread-safe". – Marko Topolnik Jul 04 '13 at 08:58
1

You have to synchronize your reset() method also.

To make a class thread safe you have to synchronize all paths that access a variable else you will have undesired results with the unsynchronized paths.

Uwe Plonus
  • 9,803
  • 4
  • 41
  • 48
  • How do you know what results he desires? Why do you assume he doesn't care if calling `reset` and `increment` at the same time gives unpredictable and inconsistent results. See my comment to Marko -- this is neither necessary nor sufficient to make the class predictable. – David Schwartz Jul 04 '13 at 08:37
0

You need to add synchronized to reset method too and then it will be synchronized. But in this way you achieve syncronization through locks, that is, each thread accesing the method will lock on the NamedCounter object instace.

However, if you use AtomicInteger as your count variable, you don't need to syncronize anymore because it uses the CAS cpu operation to achieve atomicity without the need to synchronize.

David Hofmann
  • 5,683
  • 12
  • 50
  • 78
0

Not an answer, but too long for a comment:

If reset() is synch'ed, then the 0 become visible to any thread that reads or increments the counter later. Without synchronization, there is no visibility guarantee. Looking at the interaction of concurrent increment and the unsychronized reset, it may be that 0 becomes visible to the incrementing thread before entering the method, then the result will be 1. If counter is set to 0 between increment's read and write, the reset will be forgotten. If it is set after the write, the end result will be 0. So, if you want to assert that for every reading thread, the counter is 0 after reset, that method must be synchronized, too. But David Schwartz is correct that those low-level synchronizations make little sense whithout higher-level semantics of those interactions.

Ralf H
  • 1,392
  • 1
  • 9
  • 17