CODEWITHSUNDEEP
javathread-safety
CustardBun
CustardBun

Reputation: 3867

This code isn't threadsafe - does it need to be?

I have a class, which, heavily simplified to the relevant parts, looks like this (dummy names):

@RequiredArgsConstructor
public class SomeClass {
   private final SomeProvider someProvider;
   private SomeDataStore store = null;

   private SomeDataStore getStoreAttributes() {
      if (store == null) {
          store = new SomeDataStore(someProvider, <other params>)
      }
      return store;
   }
}

A teammate commented that the check for store == null isn't threadsafe, because the value of store could change between threads. However, we're not sure if it'll matter since nothing ever sets the store to null, and I don't see a problem with multiple threads trying to set the same store to new SomeDataStore(...) because the datastore is read-only from the code.

Are there any issues I'm missing with the thread safety of this?

Thanks!

Upvotes: 0

Views: 52

Answers (3)

John Bollinger
John Bollinger

Reputation: 181459

Are there any issues I'm missing with the thread safety of this?

Yes. At least these:

  • if you have an instance of SomeClass that is shared between two or more threads, at least one of those threads ever sets the store member to a non-null value, and any other thread invokes getStoreAttributes() on that instance then you have a data race. In that case, the behavior of your program is undefined.

  • among the behaviors that can be observed in practice with a data race such as this are

    • one or more threads never observe the update to store
    • different threads observe the update to store in orders that seem mutually inconsistent

  • Even if you could assume that reads and writes to store were atomic, so that there was no data race, you still would have the issue that two different threads could both observe store to be null and enter the if block at the same time. As a result they would each instantiate a new SomeDataStore, which the method would return, defeating the apparent purpose.

Upvotes: 2

Rahul Vedpathak
Rahul Vedpathak

Reputation: 1436

In the given example, if there are multiple threads calling the method - "getStoreAttributes()", then the "store" instance variable could be initialized twice. If in your case its fine to initialize the store variable multiple times then it's fine , no need to add any thread safety. If you don't want to add thread safety features like synchronized block, at least make "store" variable volatile which will ensure that the threads will read it's latest state from the memory.

But from the name "store" looks like some resource and initializing it multiple times for no reason doesn't seem correct. And you could save that by adding thread safety. If one thread is initializing the resource, other will wait and use the initialized resource. You can implement double locking to ensure the same.

Upvotes: 1

Roy Shahaf
Roy Shahaf

Reputation: 494

The short answer is that you should strive to write your code in a way that will make it thread-safe. Now for the long answer: as far as you can tell your code doesn’t have issues right now due to this unsafe operation. Assuming you are a prodigy and have made the right assessment concerning the current situation, by allowing this into your code base you’ve added some form of debt, some piece of code that could easily prove problematic in the future and may create difficult to find bugs. It’s hard to tell but I see you’re trying to create a cached field, there are ways to make this thread safe.

Upvotes: 0

Related Questions