CODEWITHSUNDEEP
javaspringconcurrencysynchronizationthread-safety
kaqqao
kaqqao

Reputation: 15489

Are there concurrency concerns when one threads initializes the value and other threads just read it

Let's suppose I have a shared object like this:

class Shared {

   private Value val;

   //synchronized set
   public synchronized void setValue(Value val) {
      if (this.val == null) {
         this.val = val;
      } else {
          throw new IllegalStateException();
      }
   }

   //unsynchronized get
   public Value getValue() {
        return this.val;
   }
}

If I have a single thread set the value during app initialization, before any other thread has a chance to read it, and nothing ever changes the value again, is it safe to read the value unsynchronized, or do I run the risk of other threads never seeing the set value (because the variable isn't volatile and not guaranteed to be flushed to the main memory)? Imagine this in the context of a web application where the setting occurs during Servlet initialization. I do not know whether other threads have been created or not at this point, as this is the container's job. But I presume a thread pull that will handle future requests will have been created by then.

If unsafe, is there a way I'm missing to safely initialize the value without paying a price on every read forever even though the value will never change? I.e. is there a way to flush the value once only?

Also, isn't this exactly what e.g. Spring does all the time? While the container is being initialized, all kinds of unsynchronized setting on singletons is happening: beans getting injected via setters, @PostConstruct initializers firing etc. Once it is done, requests are being accepted and no modifications take place. If this was unsafe, wouldn't every singleton method ever need to be synchronized?

Upvotes: 2

Views: 356

Answers (4)

Lew Bloch
Lew Bloch

Reputation: 3433

If there's a way to initialize the element in the constructor, and make it both final and immutable, then the end of the constructor establishes a happens-before relationship with all threads that are started after the object is constructed, and the reads will be thread safe. If you initialize the element any time after construction, or if it is either not final or not immutable, then you must synchronize access. volatile will protect you against changes in the pointer being invisible, but it is not enough to make the referenced object thread safe.

Upvotes: 0

Sotirios Delimanolis
Sotirios Delimanolis

Reputation: 280175

It depends.

There are a lot of actions between you writing the value and the threads reading it that might create the happens-before relation you need to guarantee the value is present.

I'll address Spring MVC's (and really any Servlet application) situation. Spring MVC typically creates two ApplicationContext instances: one within a ContextLoaderListener and one within a DispatcherServlet.

On your typical Servlet container, the ContextLoaderListener and DispatcherServlet will be initialized sequentially, on the same thread. Then, the container will start threads to listen for connections and serve requests.

On atypical containers, you can still rely (Servlet specification, see 2.3.2 Initialization chapter) on the fact that the ContextLoaderListener and DispatcherServlet must be initialized fully (and therefore your Spring context) before it can receive requests.

When the initialization is complete, the container will either start the serving threads, and since

A call to start() on a thread happens-before any actions in the started thread.

your value will be made visible. Otherwise, the initializing thread will notify the serving threads through some other mechanism (maybe a CountDownLatch) which provides its own happens-before relation.

Assuming you only set that value from one thread and you never change it again, you don't even need the synchronized on the setter.

Look for these happens-before relations and you'll be fine. Obviously, if you don't want to, then the volatile solution is fine. If your application logic is similar to a Servlet container's (or is a Spring MVC application), you don't need that synchronized or an additional volatile. The critical piece is

before any other thread has a chance to read it, and nothing ever changes the value again

To prevent other threads from reading the value, you likely have a notification mechanism that already adds that happens-before relation that will properly publish your value.

Upvotes: 2

yshavit
yshavit

Reputation: 43401

This is a data race, so it's not safe.

It's a data race because you have two threads, one of which writes a value and the other of which reads it, without synchronization between them 1. When you're using the synchronized keyword, the synchronization comes when one thread acquires the lock after the first thread has released it 2.

Since the getValue() method is not synchronized (and thus never acquires the lock), there is no synchronization between it and any setValue(...) call. You could also provide the data synchronization by marking the field as volatile: a read from a volatile field synchronizes-with any previous write to it, 3 as well as other techniques. A full accounting of thread-safe publication is too broad for an answer here.

Because you have a data race, values are published from one thread to another non-atomically. Let's say you did:

Value v = new Value();
v.setName("Bond");
v.setId(7);
Shared.setValue(v);

It'd be possible for someone to then call getValue() and see an object whose name is "Bond" but whose id is still the default field value (0). It's even possible for it to see a null name and an id of 7, even though the code would seem to suggest that if the id is set then the name must have been. Data races leave you up to a host of subtle, un-intuitive bugs.

1. JLS 17.4.5: "When a program contains two conflicting accesses (§17.4.1) that are not ordered by a happens-before relationship, it is said to contain a data race."

2. JLS 17.4.4 "An unlock action on monitor m synchronizes-with all subsequent lock actions on m".

3. Also 17.4.4

Upvotes: 0

Bohemian
Bohemian

Reputation: 425318

It's not safe.

You need to make the field volatile:

private volatile Value val;

Other threads are allowed to cache their own copy of non-volatile fields, so changes them by one thread may not be "seen" by other threads.

volatile forces all threads to immediately "see" changes to the field (actually ,it forces threads to not use a cached copies).

Upvotes: 0

Related Questions