Does partial thread-safety make a Java class thread-safe?

Question

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

Swelling

Adding the volatile keyword to count would make it threadsafe. – Szeged

Also, your lock must be final. – Szeged

@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. – Wingover

Partial thread-safety makes the class partially thread-safe, whatever what might mean, but that's not really your question. – Tobietobin

@Swelling Where did you find this example? It would be interesting to know the source. – Thornton

Answer 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.

Thornton

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. – Lovesick

@Lovesick 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? – Thornton

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. – Lovesick

@Lovesick 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. – Thornton

i'm not sure i followed that comment at all. but, i think the argument is pretty much moot. – Lovesick

@jtahlborn: Actually, the caller thread CAN do something in order to get "consistent" results. The caller can invoke getValue() from within a synchronized block of code, after obtaining the lock of an instance of Account. No other threads can then call decrement() or increment() on that instance, until the caller thread has finished reading the current value returned by getValue() and released the lock upon leaving the synchronized block of code. – Swelling

@Swelling - sorry, the lock you are synchronizing on externally is different than the lock used in the Account class, and therefore there are no consistency guarantees. there is no way any code outside of the Account class can get and hold the lock on the Account class (as currently coded) since it is a private instance. – Lovesick

Answer 2

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-safety¹ 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 code² 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:

• http://blogs.msdn.com/b/ericlippert/archive/2009/10/19/what-is-this-thing-you-call-thread-safe.aspx

---

^{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".}

Wingover

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! – Swelling

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? – Swelling

@Swelling 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. – Thornton

Answer 3

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.

Ghiselin

Answer 4

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.

Piscine

Answer 5

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.

Damek

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. – Ghiselin

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. – Damek

Answer 6

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.

Crupper

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. – Crupper

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. – Swelling

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. – Crupper

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. – Damek

value++ is not necessarily atomic – Rebel