2

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?

kaqqao
  • 12,984
  • 10
  • 64
  • 118
  • It's not a silly question, and no, it's not safe, unless `Value` is properly immutable. – shmosel Jan 03 '17 at 23:14
  • @shmosel Because of the reason I listed or something else? Even if the value is never touched again? – kaqqao Jan 03 '17 at 23:16
  • Because other threads may never see the value, or worse, they may see it not fully constructed. – shmosel Jan 03 '17 at 23:18
  • When you say "before any other thread has a chance to read it," do you mean before any other threads are *started*? – shmosel Jan 03 '17 at 23:20
  • Will you accept "don't do it" as an answer? Because those are going to be the best answers. Or are you looking for someone to say it's OK to do? – weston Jan 03 '17 at 23:39
  • Can you back up that Spring claim? – weston Jan 03 '17 at 23:42
  • 1
    Describe _before any other thread has a chance to read it_. How do you prevent other threads from reading it? – Sotirios Delimanolis Jan 03 '17 at 23:42
  • @weston Of course I will. But not without an explanation more detailed than "don't". How are Spring singletons initialized if this is unsafe? – kaqqao Jan 03 '17 at 23:43
  • I asked if you can back it up. I'll assume that's not true about Spring until you can show the code that proves otherwise. – weston Jan 03 '17 at 23:44
  • @weston Umm, `@PostConstruct` does it. Every bean wired via a setter does it. – kaqqao Jan 03 '17 at 23:45
  • 1
    OK, Spring fields like that should be marked `volatile` if you need them to be threadsafe: http://stackoverflow.com/a/23992532/360211 – weston Jan 03 '17 at 23:47
  • @weston You can literally look at any Spring managed singleton ever. No synchronized methods anywhere, ever. No requirement to initialize final fields only. No problems. Must have an explanation. – kaqqao Jan 03 '17 at 23:51
  • "You can literally look at any Spring managed singleton ever." I'm not familiar with Spring, so I wouldn't know where to start to look. If it's so easy to find, can't you post an example of what you mean? – weston Jan 03 '17 at 23:53
  • 2
    @weston See the update in the answer you linked. He says Spring beans are generally thread-safe out of the box, and explains why. – shmosel Jan 03 '17 at 23:55
  • @shmosel that should totally be at the top of that answer as a TL;DR! – weston Jan 03 '17 at 23:57
  • @weston Your link is actually describing exactly what I wanted to know!! Spring actually **does** have a nifty way to avoid issues, and volatile is very much not mandatory in many cases, as a result. Brilliant find. Thank you!! Post it as an answer and I'll accept it. – kaqqao Jan 03 '17 at 23:58
  • I think the problem is that this question is asking two pretty different questions. The first is, "given this example class, and given no control over how/when threads are created, is this enough to be safe?" The answer there is no, even if you don't like that. :-) The second question -- which was tacked on later, I might add -- is whether Springs add stuff on top of it to make it be safe. – yshavit Jan 04 '17 at 00:11
  • @yshavit True. It did take me a while to fully formulate the question, and mention the web app initialization context, which changes the answer. But that is only because I was confused myself. I'll update the question to properly communicate what I meant to say. Sorry for this! – kaqqao Jan 04 '17 at 00:44

4 Answers4

2

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.

Sotirios Delimanolis
  • 274,122
  • 60
  • 696
  • 724
  • Finally. An actual explanation! Thank you endlessly, good sir! – kaqqao Jan 04 '17 at 00:07
  • The mind-blowing part is that there's no explicit happens-before guaranteed by the Servlet spec between the Servlet's `init()` and `service()`. Still, as nicely explained in http://stackoverflow.com/questions/11719916/marking-servlet-instance-variables-defined-in-init-as-volatile even the `GenericServlet` implementation seems to simply assume it's there. – kaqqao Jan 04 '17 at 01:59
0

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

yshavit
  • 42,327
  • 7
  • 87
  • 124
  • I think I get it, is another way of saying this is the compiler and the VM are both allowed to reorder operations outside of memory barriers (for optimisation etc), and `setValue` could happen before `setId`? – weston Jan 03 '17 at 23:32
  • Reads are guaranteed to occur a lot later than the writing. The set is done before the app will accept requests (framework initialization code). There's no way for the two to occur simultaneously. Does this change anything? – kaqqao Jan 03 '17 at 23:33
  • @kaqqao In practice, it makes it more likely that you'll see the Value in full. But "more likely" is not guaranteed. What _is_ guaranteed is that if you ever get unlikely and see a partial update like this, it'll be extremely hard to debug. :) If the `setValue()` calls happen before the reader threads even start (which is going to be hard or impossible to guarantee if you're using an ExecutorService), for instance, then you are guaranteed to see Value in full. But you're getting into pretty subtle territory, for relatively little gain. As Bohemian said, you can also make the field volatile. – yshavit Jan 03 '17 at 23:44
  • @weston Thanks :) And I missed your first question to my answer, but yes, that's it exactly. Between reordering and memory cache flushing, you can see operations happen in pretty much any order unless you tell java otherwise -- and synchronization (including volatile fields, etc) is how you do that. – yshavit Jan 03 '17 at 23:55
  • 1
    My issue with your answer is that it ignores _before any other thread has a chance to read it, and nothing ever changes the value again_ which was bold in the original revision. You're rehashing general information about happens-before and visibility but not adapting it to their question. How does OP enforce _before any chance to read it_? That's critical. There are quite a few things that can cause correct program order without controlling when threads are started. – Sotirios Delimanolis Jan 04 '17 at 01:38
  • @SotiriosDelimanolis When I first answered it, the question didn't have the Spring stuff, so I was addressing what was there. As the "before any other thread has a chance to read it" -- that can be established in lots of ways that don't establish a happens-before, and in the absence of explicit synchronization, they would not be safe. The "nothing ever changes the value again" bit is irrelevant if the initial publication itself is unsafe. – yshavit Jan 04 '17 at 02:11
  • One trivial way you could get that unsafe behavior: you have one API that ends up in the `setValue(...)` path, and other APIs that end up with the `getValue()` paths. When I start the web server, my service start script starts the JVM, calls the setValue API, and only then publicly exposes the service (say, by updating a proxy), including non-startup API calls that use `getValue()`. – yshavit Jan 04 '17 at 02:12
  • That is a critical part of the question (even without Spring). Edit your answer to address that possibility (and others) and you'll have improved it. – Sotirios Delimanolis Jan 04 '17 at 02:39
0

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

Bohemian
  • 412,405
  • 93
  • 575
  • 722
  • Kind of suggests `volatile` is the only way (and they are half way to a `synchronized` solution). Also, I think it over simplifies `volatile`s role. Doesn't it prevent compiler and VM reordering operations for example? – weston Jan 03 '17 at 23:37
  • @weston it forces a "happens before" relationship: all writes that a thread makes to anything before writing to a volatile are seen by other threads after they read the volatile. – Bohemian Jan 03 '17 at 23:39
0

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.

Lew Bloch
  • 3,364
  • 1
  • 16
  • 10