Java FutureTask - Multithreaded call to get()

Question

I have the following two methods in a class:

private MyDef myDef;
private FutureTask<MyDef> defFutureTask;

public synchronized void periodEviction() {
       myDef = null;
}

    public MyDef loadMyItems() {

    // if it's not ready use a future - it will block until the results are ready
    if (this.myDef == null) { // this will still not be thread safe
        Callable<MyDef> callableDef = ()->{ return this.loadFromDatabase(); };
        FutureTask<MyDef> defTask = new FutureTask<>(callableDef);
        this.defFutureTask = defTask;
        defFutureTask.run();            
    }        

    try {
        // wait until's it's ready
        this.myDef = this.qDefFuture.get();                     
    } catch(InterruptedException e) {
        log.error(this.getClass(), "Interrupted whilst getting future..");
    } catch(ExecutionException e) {
        log.error(this.getClass(), "Error when executing callable future");
    }         
    return this.myDef; 
}

I wanted to do the following:

1) Do a cache eviction using periodEviction() every one hour or so.

2) Otherwise, use the cached value when db loading is done.

I believe I have misunderstood Java future as I couldn't answer the question, "What happens when Thread A,B,and C all are calling loadMyItems() at the same time?"

So does this mean without something like an executor, this implementation is still not thread safe?

What happens is that three threads will start the asynchronous query, each will return its result, and the last one to set the value will be in the cache for the next hour. To make it thread safe, make `loadMyItems` into `synchronized` as well. — daniu, Jun 28 '18 at 09:01

cobbzilla · Answer 1 · 2018-06-28T10:34:24.950

A super simple approach would be to declare loadMyItems as synchronized. But if the class has other methods that access myDef, you would have to declare those synchronized too. Sometimes this results in very coarse-grained locking and slower performance.

If you're looking for the cleanest/fastest code, instead of declaring periodEviction as synchronized, declare myDef as an AtomicReference:

private final AtomicReference<MyDef> myDef = new AtomicReference<>();

Then the body of periodEviction is:

synchronized (myDef) {
    myDef.set(null);
}

And the body of loadMyItems is:

synchronized (myDef) {
   if (myDef.get() == null) {
        // perform initialization steps, ending with:
        myDef.set(this.qDefFuture.get());
   }
   return myDef.get();
}

If many threads call loadMyItems at the same time, myDef will only ever be initialized once, and they will all get the same object returned (unless somehow a call to periodEviction snuck in the middle).

score 1 · Accepted Answer · answered Jun 28 '18 at 09:23

An even simpler approach is to not cache the object at all but just retain the Future.

private CompletableFuture<MyDef> defFuture;

public synchronized void periodEviction() {
    // evict by triggering the request anew
    defFuture = CompletableFuture.supplyAsync(this::loadFromDatabase);
}

public synchronized Optional<MyDef> loadMyItems() {
    try {
        return Optional.of(this.defFuture.get());
    } catch(InterruptedException e) {
        log.error(this.getClass(), "Interrupted whilst getting future..");
    } catch(ExecutionException e) {
        log.error(this.getClass(), "Error when executing callable future");
    }         
    return Optional.empty();
}

With the caveat that this will trigger the database query every eviction period rather than on demand.

Java FutureTask - Multithreaded call to get()

2 Answers2