5

I came across the example below of a Java class which was claimed to be thread-safe. Could anyone please explain how it could be thread-safe? I can clearly see that the last method in the class is not being guarded against concurrent access of any reader thread. Or, am I missing something here?

public class Account {
    private Lock lock = new ReentrantLock();
    private int value = 0;
    public void increment() {
       lock.lock();
       value++;
       lock.unlock();
    }
    public void decrement() {
       lock.lock();
       value--;
       lock.unlock();
    }
    public int getValue() {
       return value;
    }
}
5
  • Adding the volatile keyword to count would make it threadsafe.
    – Jazzwave06
    Jul 10, 2015 at 16:09
  • Also, your lock must be final.
    – Jazzwave06
    Jul 10, 2015 at 16:09
  • @sturcotte06 - Yes ... but both comments are beside the point. This is not about how to write the code correctly. It is about the specific properties of this version, and whether it is "thread-safe" ... and what that actually means.
    – Stephen C
    Jul 10, 2015 at 23:48
  • Partial thread-safety makes the class partially thread-safe, whatever what might mean, but that's not really your question.
    – user207421
    Jul 11, 2015 at 0:35
  • @softwarelover Where did you find this example? It would be interesting to know the source. Jul 11, 2015 at 14:24

6 Answers 6

6

The code is not thread-safe.

Suppose that one thread calls decrement and then a second thread calls getValue. What happens?

The problem is that there is no "happens before" relationship between the decrement and the getValue. That means that there is no guarantee, that the getValue call will see the results of the decrement. Indeed, the getValue could "miss" the results of an indefinite sequence of increment and decrement calls.

Actually, unless we see the code that uses the Account class, the question of thread-safety is ill-defined. The conventional notion of thread-safety1 of a program is about whether the code behaves correctly irrespective of thread-related non-determinacy. In this case, we don't have a specification of what "correct" behaviour is, or indeed an executable program to test or examine.

But my reading of the code2 is that there is an implied API requirement / correctness criterion that getValue returns the current value of the account. That cannot be guaranteed if there are multiple threads, therefore the class is not thread-safe.

Related links:


1 - The Concurrency in Practice quote in @CKing's answer is also appealing to a notion of "correctness" by mentioning "invalid state" in the definition. However, the JLS sections on the memory model don't specify thread-safety. Instead, they talk about "well-formed executions".

2 - This reading is supported by the OP's comment below. However, if you don't accept that this requirement is real (e.g. because it is not stated explicitly), then the flip-side is that behaviour of the "account" abstraction depends on how code outside of the Account class ... which makes this a "leaky abstraction".

3
  • Exactly! That is what I wanted to see someone write. We can imagine a joint-account being accessed by two partners and making decisions (withdraw or deposit?) based on the amount they see (getValue) on their two distinct ATM screens (perhaps, in two different cities)! A partner may see $10 but by the time his thread wants to withdraw $5, another partner's thread may have withdrawn $10 already, causing the account balance to be -$5! The risk is there in the current definition of the class! Jul 10, 2015 at 15:23
  • As an example, is this why the designers of StringBuffer in the Java standard API called that class thread-safe, without knowing how (thread-safe ways vs non-thread-safe ways) application developers would use it? Jul 10, 2015 at 15:41
  • @softwarelover The class is said to be thread-safe. See my answer for an example of a JDK class that is said to be thread-safe and yet required client code to ensure thread safety. It resonates the sentiments of your question. That being said, any usage of the class in other classes may not be thread safe as pointed out in this answer. Jul 10, 2015 at 18:27
2

This is not thread safe purely due to the fact there is no guarantees about how the compiler can re-order. Since value is not volatile here is your classic example:

while(account.getValue() != 0){

}

This can be hoisted to look like

while(true){
  if(account.getValue() != 0){

  } else {
      break;
  }
}

I can imagine there are other permutations of compiler fun which can cause this to subtly fail. But accessing this getValue via multiple threads can result in failure.

2

There are several distinct issues here:

Q: If multiple threads make overlapped calls to increment() and decrement(), and then they stop, and then enough time passes with no threads calling increment() or decrement(), will getValue() return the correct number?

A: Yes. The locking in the increment and decrement methods insures that each increment and decrement operation will happen atomically. They can not interfere with one another.


Q: How long is enough time?

A: That's hard to say. The Java language specification does not guarantee that a thread calling getValue() will ever see the latest value written by some other thread because getValue() accesses the value without any synchronization at all.

If you change getValue() to lock and unlock the same lock object or if you declare count to be volatile, then zero amount of time would be enough.


Q: Can a call to getValue() return an invalid value?

A: No, It can only ever return the initial value, or the result of complete increment() call or the result of a complete decrement() operation.

But, the reason for this has nothing to do with the lock. The lock does not prevent any thread from calling getValue() while some other thread is in the middle of incrementing or decrementing the value.

The thing that prevents getValue() from returning a completely invalid value is that value is an int, and the JLS guarantees that updates and reads of int variables are always atomic.

1

The short answer :

By definition,Account is a thread-safe class even though the geValue method is not guarded

The long answer

From Java Concurrency in practice a class is said to be thread safe when :

No set of operations performed sequentially or concurrently on instances of a thread-safe class can cause an instance to be in an invalid state.

Since the the getValue method will not result in the Account class being in an invalid state at any given time, your class is said to be thread safe.

The documentation for Collections#synchronizedCollection resonates this sentiment :

Returns a synchronized (thread-safe) collection backed by the specified collection. In order to guarantee serial access, it is critical that all access to the backing collection is accomplished through the returned collection. It is imperative that the user manually synchronize on the returned collection when iterating over it:

 Collection c = Collections.synchronizedCollection(myCollection);
 ...   
  synchronized (c) {
  Iterator i = c.iterator(); // Must be in the synchronized block
  while (i.hasNext())
     foo(i.next());   
  }

Notice how the documentation says that the collection (which is an object of an inner class named SynchronizedCollection in the Collections class) is thread-safe and yet asks the client code to guard the collection while iterating over it. Infact, the iterator method in SynchronizedCollection is not synchronized. This is very similar to your example where Account is thread-safe but client code still needs to ensure atomicity when calling getValue.

7
  • unlike the example you provided, there is no way a caller of the Account class could get a "valid" value since the locking is internal.
    – jtahlborn
    Jul 10, 2015 at 23:16
  • @jtahlborn The point I was making is that the JDK has a thread safe class that requires external locking. Classes that require external locking can still be called thread safe. Did I miss your point? Jul 11, 2015 at 3:55
  • the jdk class is thread-safe. the caller is required to do extra work in order to get "consistent" results. the Account class is "thread-safe" in the fashion that you can't screw it up. however, there is nothing the caller can do get "consistent" results, so i don't think it's a good parallel.
    – jtahlborn
    Jul 11, 2015 at 13:14
  • @jtahlborn The JDK cannot guarantee that client code will guard a SynchronizedCollection while iterating over it using an Iterator. The JDK class is partially thread-safe and is still documented as a thread-safe class. Club this with the quote from Concurrency in practice about invaid state and you have the answer to the question: Does partial thread-safety make a Java class thread-safe? It's another thing that there is nothing client code can do to get consistent results. The client code can't be thread-safe but Account can since it's state can't be made inconsistent. Jul 11, 2015 at 13:56
  • i'm not sure i followed that comment at all. but, i think the argument is pretty much moot.
    – jtahlborn
    Jul 11, 2015 at 14:17
0

It's completely thread safe.

Nobody can simultaneously increment and decrement value so you won't lose or gain a count in error.

The fact that getValue() will return different values through time is something that will happen anyway: simultaneity is not relevant.

2
  • 1
    The operation is atomic, but you can always run the issue of memory visibility. So I wouldn't say it's completely thread safe. See Stephen C's answer for an explanation.
    – John Vint
    Jul 10, 2015 at 15:20
  • I don't think that matters. Since you don't know how the threads will be dealt with by the processor(s), the simultaneity is not relevant, as I clearly state.
    – Bathsheba
    Jul 10, 2015 at 15:40
0

You do not have to protect getValue. Accessing it from multiple threads at the same time does not lead to any negative effects. The object state cannot become invalid no matter when or from how many threads you call this methid (because it does not change).

Having said that - you can write a non-thread-safe code that uses this class.

For example something like

if (acc.getValue()>0) acc.decrement();

is potentially dangerous because it can lead to race conditions. Why?

Let's say you have a business rule "never decrement below 0", your current value is 1, and there are two threads executing this code. There's a chance that they'll do it in the following order:

  1. Thread 1 checks that acc.getValue is >0. Yes!

  2. Thread 2 that acc.getValue is >0. Yes!

  3. Thread 1 calls decrement. value is 0

  4. Thread 2 calls decrement. value is now -1

What happened? Each function made sure it was not going below zero, but together they managed to do that. This is called race condition.

To avoid this you must not protect the elementary operations, but rather any pieces of code that must be executed uninterrupted.

So, this class is thread-safe but only for very limited use.

5
  • CKing's answer also covers this. No set of operations performed sequentially or concurrently on instances of a thread-safe class can cause an instance to be in an invalid state. So, if any value is valid then the code is thread-safe. But if you have any additional notions of what "valid state" is (such as it being>0, or correlated with a different object etc), then the code is not thread-safe.
    – yu_sha
    Jul 10, 2015 at 15:25
  • yu_sha, can't we then not say that the class is not thread-safe BECAUSE it can lead to race conditions? Please scroll and see my example below of a join-account. Jul 10, 2015 at 15:25
  • I think we are saying the same thing. That the class itself is thread-safe only while you are using it in a certain way and do not make any assumptions. In any case, protecting getValue will do nothing for thread-safety.
    – yu_sha
    Jul 10, 2015 at 15:28
  • You should note that the issue you cite is still prevalent even if getValue was locked: you'd need an atomic "decrement if" to achieve thread safety.
    – Bathsheba
    Jul 10, 2015 at 15:44
  • value++ is not necessarily atomic
    – kostya
    Jul 14, 2015 at 2:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.