Is this Java cache threadsafe and optimal?

Question

Can this Java cache be safer or faster?

public class Cache {
    private long lastupdateTime = 0L;
    private String cachedData = null;
    public String getData() {
        long now = System.currentTimeMillis();
        if (now - lastupdateTime < 30000) {
            return cachedData;
        }
        else {
            synchronized (this) {
                if (now - lastupdateTime < 30000) {
                    return cachedData;
                }
                else {
                    lastupdateTime = now;
                    cachedData = getDataFromDatabase();
                    return cachedData;
                }
            }
        }
    }
    private String getDataFromDatabase() { ... }
}

Answer 1

Here's an idea - on the principle that synchronized is an archaic and inefficient mechanism.

Here I use an AtomicReference<Phaser> to indicate the cache is being updated. The Phaser can be used to await completion of the update.

public class Test {

  public static class Cache {
    // Cache timeout.
    private static final long Timeout = 10000;
    // Last time we updated.
    private volatile long lastupdateTime = 0L;
    // The cached data.
    private volatile String cachedData = null;
    // Cache is in the progress of updating.
    private AtomicReference<Phaser> updating = new AtomicReference<>();
    // The next Phaser to use - avoids unnecessary Phaser creation.
    private Phaser nextPhaser = new Phaser(1);

    public String getData() {
      // Do this just once.
      long now = System.currentTimeMillis();
      // Watch for expiry.
      if (now - lastupdateTime > Timeout) {
        // Make sure only one thread updates.
        if (updating.compareAndSet(null, nextPhaser)) {
          // We are the unique thread that gets to do the updating.
          System.out.println(Thread.currentThread().getName() + " - Get ...");
          // Get my new cache data.
          cachedData = getDataFromDatabase();
          lastupdateTime = now;
          // Make the Phaser to use next time - avoids unnecessary creations.
          nextPhaser = new Phaser(1);
          // Get the queue and clear it - there cannot be any new joiners after this.
          Phaser queue = updating.getAndSet(null);
          // Inform everyone who is waiting that they can go.
          queue.arriveAndDeregister();
        } else {
          // Wait in the queue.
          Phaser queue = updating.get();
          if (queue != null) {
            // Wait for it.
            queue.register();
            System.out.println(Thread.currentThread().getName() + " - Waiting ...");
            queue.arriveAndAwaitAdvance();
            System.out.println(Thread.currentThread().getName() + " - Back");
          }
        }
      }
      // Let them have the correct data.
      return cachedData;
    }

    private String getDataFromDatabase() {
      try {
        // Deliberately wait for a bit.
        Thread.sleep(5000);
      } catch (InterruptedException ex) {
        // Ignore.
      }
      System.out.println(Thread.currentThread().getName() + " - Hello");
      return "Hello";
    }
  }

  public void test() throws InterruptedException {
    System.out.println("Hello");
    // Start time.
    final long start = System.currentTimeMillis();
    // Make a Cache.
    final Cache c = new Cache();
    // Make some threads.
    for (int i = 0; i < 20; i++) {
      Thread t = new Thread(new Runnable() {
        @Override
        public void run() {
          while (System.currentTimeMillis() - start < 60000) {
            c.getData();
            try {
              Thread.sleep(500);
            } catch (InterruptedException ex) {
              // Ignore.
            }
          }
        }
      });
      t.setName("Thread - " + i);
      t.start();
      // Stagger the threads.
      Thread.sleep(300);
    }
  }

  public static void main(String args[]) throws InterruptedException {
    new Test().test();
  }
}

Please note that this is the first time I have used a Phaser - I hope I have it right.

Note that this algorithm may break down if the timeout is longer than the time it takes to get the data from the database.

Answer 2

You have your Cash class with two shared *mutable* state lastupdateTime and cachedData, getData() can be invoked conurrenctly by two different threads, it use a local variable now that can be visible to other threads (alocated in the stack)

What's wrong ?

• First if (now - lastupdateTime < 30000) { return cachedData;}, Other thread can see stale data on lastupdateTime and cachedData (you are accessing to a thared mutable state without synchronization guarantee)

• now variable form such a invariant with lastupdateTime, so now - lastupdateTime should be executed in an Atomic Block

What can I do ?

Just make method synchronized, and make your class totally thread-safe

public class Cache {
    private long lastupdateTime = 0L;
    private String cachedData = null;
    public synchronized String getData() {
           long now = System.currentTimeMillis()
           if (nano - lastupdateTime < 30000) {
                return cachedData;
           }
           else {
               lastupdateTime = now;
               cachedData = getDataFromDatabase();
               return cachedData;
           }                
    }
    private String getDataFromDatabase() { ... }
}

Answer 3

Yes. Even ignoring the fact that you haven't made lastupdateTime or cachedData volatile,

                lastupdateTime = now;
                cachedData = getDataFromDatabase();

is out of order.
If getDataFromDatabase fails with an exception, you will have already updated lastupdateTime , so will keep returning stale data for 30s, possibly returning null if getDataFromDatabase fails on the first attempt.

You would lose nothing by initializing lastUpdateTime to Long.MIN_VALUE, and would work more reliably on systems whose clocks have been set backwards.

System.getCurrentTimeMillis can go backwards which is unlikely to affect a 30s cache, but for this use case there's really no reason not to use System.nanoTime() instead.