accessing a String across multiple threads

Question

I'm looking for some input here. I have a singleton class that contains a value which is updated every few seconds by a method within that class. Right now, access to this value across multiple threads is done via synchronization, which I would like to eliminate. Would this make sense?

class DataSegment {

    private MetricsUpdater metrics = new MetricsUpdater.getInstance();

    public String printValues() {
        StringBuilder sb = new StringBuilder();
        sb.append(value1);
        sb.append(morevalues);
        sb.append(metrics.myValue); // this is the value that's currently synchronized
        return sb.toString();
    }
}


class MetricsUpdater {

    private String myValueSynchronized;
    public String myValue;

    public static MetricsUpdater getInstance() {
        if (theInstance == null) {
            theInstance = new MetricsUpdater();
        }
        return theInstance;
    }

    // this runs on a timer but to keep it simple I'll just define the method...
    private void updateMetrics() {

        synchronized(myValue) {
            // also, to keep things simple, I've replaced all of the actual logic with a method called someMethodToUpdateMyValue()
            myValueSynchronized = someMethodToUpdateMyValue();
            myValue = myValueSynchronized;
        }
    }
}

There can be many instances of DataSegment all reading from myValue, but the metrics class is a singleton. myValue only updates every 5 seconds or so and only MetricsUpdater is allowed to write to it. Does that make sense?

Does it even need to be synchronized at all if all of the other threads are only allowed to read it? I've run a boatload of JUnit tests on this, creating many instances of the DataSegment class all printing values like crazy and I have yet to see any concurrency issues.

Answer 1

There are some problems with your code.

1st Problem

synchronized(myValue) {
    myValueSynchronized = someMethodToUpdateMyValue();
    myValue = myValueSynchronized;
    Thread.sleep(100); 
}

your critical section is wrong because are taking lock on myValue. Suppose you put a Thread.sleep(100) before exiting critical section. Then it means other thread will take a lock on new myValue instance and thus can enter the critical section. if its a time thread and if its frequency is very high. Then you can have stale updated overriding the new ones. Anyways its a bad Practice to acqurie lock on such monitors. Use ReentrantLock or synchronize on some final reference of String.

2nd Problem

    public static MetricsUpdater getInstance() {
        if (theInstance == null) {
            theInstance = new MetricsUpdater();
        }
        return theInstance;
    }

Your Singleton code is broken. Use DCL (Double Checked Locking see below in my solution sec). Or Use private static MetricsUpdater theInstance = new MetricsUpdate();. Latter is better,

3rd Problem

 sb.append(metrics.myValue); 

The above code should be called in a synchronized context or declared as volatile. Latter is better

Solution 1 - Assuming someMethodToUpdateMyValue is thread safe

class MetricsUpdater {

    private static volatile MetricsUpdater theInstance;
    public volatile String myValue;

    /**
     * DCL . Please avoid
     * Better use 
     * private static MetricsUpdater theInstance = new MetricsUpdate();
     */
    public static MetricsUpdater getInstance() {
        if (theInstance == null) {
            synchronized(MetricsUpdate.class) {
                 if(theInstance == null) {
                     theInstance = new MetricsUpdater();
                 }
            }
        }
        return theInstance;
    }

    // this runs on a timer but to keep it simple I'll just define the method...
    // if your someMethodToUpdateMyValue is thread safe
    private void updateMetrics() {
            myValue = someMethodToUpdateMyValue();
    }
}

Solution 2 : Assuming someMethodToUpdateMyValue is not Thread Safe

No need of synchronization is reference read/write is atomic and we have delared myValue as volatile

class MetricsUpdater {

 private static volatile MetricsUpdater theInstance;
 public volatile String myValue;

 /**
 ** Use ReentrantLock instead
 */
 private final  Object lock  = new Object();


     /**
     * DCL . Please avoid
     * Better use 
     * private static MetricsUpdater theInstance = new MetricsUpdate();
     */
    public static MetricsUpdater getInstance() {
        if (theInstance == null) {
            synchronized(MetricsUpdate.class) {
                 if(theInstance == null) {
                     theInstance = new MetricsUpdater();
                 }
            }
        }
        return theInstance;
    }

// this runs on a timer but to keep it simple I'll just define the method...
private void updateMetrics() {
    synchronized(lock) {
        myValue = someMethodToUpdateMyValue();
    }
}

}

Answer 2

It does need to be synchronized or the variables being read by multiple threads need to be marked as volatile (or anything else that causes java to flush the variable value). The java memory model does not guarantee that one thread will (ever) see the value of variable written by another thread. In practice, the values are often seen by multiple threads correctly, but if you want to ensure it, you must properly synchronize (or use volatile/locks/etc) to ensure the value is flushed.