Question

I have a server somewhat like this:

class Server {

    private WorkingThing worker;

    public void init() {
         runInNewThread({
             // this will take about a minute
             worker = new WorkingThing();
         });
    }

    public Response handleRequest(Request req) {
         if (worker == null) throw new IllegalStateException("Not inited yet");
         return worker.work(req);
    }

}

As you can see, there are threads handling requests and a thread initing the server. Requests can come in before initing has finished, therefore there is the check with an IllegalStateException.

Now, to make this thread safe (so request handler threads don't see a stale, null-valued, version of worker just after init), I'd have to make worker volatile, synchronize on it, or some such.

However, after the init has completed, worker won't ever change again, so it's effectively final. Therefore, it seems like any lock contention that may occur would be a waste. So, what's the most efficient thing I can do here?

Now I know it doesn't really matter in a practical sense (with all the heavy lifting of reading a network request, etc, what does a single lock matter?), but I'd like to know out of curiosity.

Was it helpful?

Solution

A note on volatile: marking a variable volatile is cheaper than using synchronization (it does not involve locks) and is generally cheap enough that you won't notice. In particular, on x86 architectures, reading a volatile variable does not cost more than reading a non-volatile variable. However writing to a volatile is more expensive and the fact that the variable is volatile might prevent some compiler optimisations.

So using volatile is probably the option that gives you the best performance/complexity ratio in your scenario.

You don't have that many alternatives. In the end it boils down to ensuring a safe publication of your worker. And safe publication idioms include:

  • initialising the instance from a static initialiser
  • marking the reference to the instance as final
  • marking the reference to the instance as volatile
  • synchronizing all accesses

In your case, only the last two options are available and using volatile is more efficient.

OTHER TIPS

You should declare the worker is volatile.

Because of two reason

  1. if you don't declare it as volatile than due to reordering and visibility effects you could see non-null reference to incomplete constructed Worker Object. and thus you could have undesirable effects. This would not be happen if you make your worker immutable using all final variables.

  2. In theory it could be possible that your main thread may not see non-null reference of your worker object for a long time. So avoid it.

So concluding if worker is immutable than also for point 2 you should make it volatile. Always avoid unpredictable results. Declaring it volatile will take care of these issues.

Use simple locking for the initial request:

public synchronized void init() {
    if(worker!=null) return;
    runInNewThread({
        synchronized(Server.this){
            worker = new WorkingThing();
            Server.this.notify();
        }
    });
    this.wait();
}

public Response handleRequest(Request req) {
    if (worker == null) synchronized(this) {
        this.wait();
    }
    return worker.work(req);
}

This works because there are synchronization points between accesses to worker.

I'd conisder using ava.util.concurrent.atomic.AtomicReference if it is null, iether throw or wait and try again (if init is fast enough).

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top