2
\$\begingroup\$

I wanted to use an async-cache to store URLs of images that I have to display in a list. The image URL is fetched (REST call) using a unique UUID associated with each item of the list. I store this URL locally and then use it to show images in the future.

I came up with the following async-cache to make my life easier.

/**
 * A basic cache which loads data in background and returns <Status, Data>
 *     Status : LOADING, ERROR, FOUND
 *     Data : the data requested
 *
 * @param <T> the Data to cache
 * @param <E> the parameter which can be used to uniquely fetch that data (network url for eg)
 */

public class AsyncCache<E, T> {

    //fetches data in background
    private final ExecutorService executor;
    {
        //Test maximum number of threads
        final int CPU_COUNT = Runtime.getRuntime().availableProcessors();
        final int CORE_POOL_SIZE = CPU_COUNT + 1;
        final int MAXIMUM_POOL_SIZE = CPU_COUNT * 2 + 1;
        final int KEEP_ALIVE = 1;
        /**
         * An {@link Executor} that can be used to execute tasks in parallel.
         */
        executor = Executors.unconfigurableExecutorService(new ThreadPoolExecutor(CORE_POOL_SIZE, MAXIMUM_POOL_SIZE, KEEP_ALIVE, TimeUnit.SECONDS, new LinkedBlockingQueue<>(), (r, executor) -> {
            Log.i("Ayush", "Cache rejected my runnable :(");
        }));
//        executor = Executors.newSingleThreadExecutor(); //for testing
    }

    //caches found data
    final ConcurrentHashMap<E, T> mainCache = new ConcurrentHashMap<>();
    //caches Java futures
    final ConcurrentHashMap<E, Future> futureCache = new ConcurrentHashMap<>();

    //function to load T (data) using E (key)
    final Function<E, Optional<T>> loadData;
    //callback to get key for data
    final KeyCallback<E, T> keyCallback;
    //empty values to cache (prevent useless re-trials)
    final EmptyDataCallback<T> emptyDataCallback;

    public AsyncCache(Function<E, Optional<T>> loadData, KeyCallback<E, T> keyCallback, EmptyDataCallback<T> emptyDataCallback) {
        this.loadData = loadData;
        this.keyCallback = keyCallback;
        this.emptyDataCallback = emptyDataCallback;
    }

    /**
     * The get call. Uses key to fetch the data in background and uses Result Callback to
     * pass that data to caller, if hes alive (using weakReferences)
     * @param key the key for the data
     * @param doWork result callback
     * @return data bundled with status
     */
    public final DataReturn getData(E key, ResultCallback<T> doWork) {

        final T fromMainCache = mainCache.get(key);

        if (fromMainCache != null) {

            //data found in main cache, ez life
            return new DataReturn(Status.FOUND, fromMainCache);
        } else {

            final Future fromLoader = futureCache.get(key);
            if (fromLoader == null) {

                //data not found and not loading, lets load it
                futureCache.put(key, executor.submit(new DataLoader(key, doWork)));
                return new DataReturn(Status.LOADING, null);
            } else {

                /**
                 * Data not found, but probably loading.
                 * We are confident that "execution exception" will not be thrown
                 */
                if (fromLoader.isCancelled() || fromLoader.isDone()) {
                    //restart load ?
                    return new DataReturn(Status.ERROR, null);
                } else
                    return new DataReturn(Status.LOADING, null);
            }
        }
    }

    public final void submitDataExplicitly(T data) {

        final E key = keyCallback.getKey(data);
        mainCache.put(key, data);

        //remove any running thread for same data
        final Future future = futureCache.get(key);
        if (future != null)
            future.cancel(true); //the thread will handle removal
    }

    public interface KeyCallback<E, T> {
        E getKey(T data);
    }

    public interface EmptyDataCallback<T> {
        T getEmptyData();
    }

    public interface ResultCallback<T> {
        void result(Optional<T> data);
    }

    public enum Status {
        LOADING,
        ERROR,
        FOUND
    }

    /**
     * Packaging for data retrieval from cache.
     * Will also tell the status.
     * ALWAYS CHECK STATUS !
     */
    public final class DataReturn {

        private final Status status;
        private final T data;

        private DataReturn(Status status, T data) {
            this.status = status;
            this.data = data;
        }

        public Status getStatus() {
            return status;
        }

        public Optional<T> getData() {

            if (status == Status.LOADING || status == Status.ERROR)
                return Optional.absent();
            if (status == Status.FOUND)
                return Optional.of(data);
            else
                throw new IllegalStateException("AsyncCache illegal status exception, status can not be " + status);
        }
    }

    /**
     * Loads data in background.
     * Holds a weakReference to resultCallback to pass result
     */
    private final class DataLoader implements Runnable {

        final E key;
        final WeakReference<ResultCallback<T>> resultReference;

        private DataLoader(E key, ResultCallback<T> result) {
            this.key = key;
            this.resultReference = new WeakReference<>(result);
        }

        @Override
        public void run() {

            final Optional<T> result;
            try {
                result = loadData.apply(key);
            } catch (Exception e) {
                //if anything goes wrong, we MUST remove from future cache
                e.printStackTrace();
                futureCache.remove(key);
                mainCache.putIfAbsent(key, emptyDataCallback.getEmptyData()); //we do not want useless retrials
                return;
            }

            //remove from future cache
            futureCache.remove(key);

            if (result != null && result.isPresent()) {

                //save in main cache
                mainCache.put(key, result.get());
                final ResultCallback<T> resultCallback = resultReference.get();
                if (resultCallback != null)
                    resultCallback.result(result);
                else
                    Log.i("Ayush", "Result reference expired");

            } else
                mainCache.putIfAbsent(key, emptyDataCallback.getEmptyData()); //we do not want useless retrials
        }
    }
}
\$\endgroup\$
2
\$\begingroup\$

This is an interesting class. When I read it the first time, I thought it is very powerful, but the second time I noticed that the class is doing more than one thing. It handles the loading of resources, it handles the concurrent execution, and it has a cache inside. It is interesting, because, if you extract the resource loading, you only have got a wrapper around your mainCache map, so this is a resource loader.

You have allot of inner classes and interfaces.

What is the use case of KeyCallback? You said you use this class for caching images, so this would be a AsyncCache<URL, BufferedImage>. Now, you have some stored some images on your device and want to fill your cache with those images. So you have to hold a mapping between the BufferedImages and the local URLs to get the right key when the cache asks for it by providing the image. Isn't easier to put the image with its key without this detour? Like this: public final void submitDataExplicitly(E key, T data)?

And EmptyDataCallback? If this is to get a default value, then you should provide a context like getEmptyData(E key). Maybe you can get rid of this interface by caching DataReturns and by extending the Status by a NOT_FOUND.

The return value of DataReturn::getData() is redundant to the status of a DataReturn. You have to check for the status and (if the status is FOUND) you have to check the optional, but your DataReturn is something like an Optional. If you just return data if the status is FOUND and throwing an exception otherwise, you force the user of the class to check the status instead of only suggesting it by writing a comment. (Maybe you are interested in Scott Meyers moste imported design guildline, IMO he is very theatrical, but he has the right ideas.)

The constructor should test the parameters. If one of the parameters is null, you'll notice it if you try to get or add a value. But this isn't the location of the fault. See fail-fast principles. And the inner class constructors should do this as well.


The naming of your variables and methods seems to be fine. I don't like the one letter generic types (I would use this class definition: AsyncCache<KeyType, ValueType>), but this is my opinion and your names are the standard.

You sometime have a newline after the beginning of a new block, sometimes you don't. You are mixing if-else-statemens with and without braces. You should be more consistent, because something like this seems to be very odd to me:

if (result != null && result.isPresent()) {
[...]
    else
        Log.i("Ayush", "Result reference expired");

} else
    mainCache.putIfAbsent(key, emptyDataCallback.getEmptyData()); //we do not want useless retrials
[...]

You declared the methods of AsyncCache as final. This is fine, because a subclass will break the functionality. But is this class meant to be subclassed? Maybe you declare the whole class as final. The class members are final as well, but not private. They should be private, so no other class could break the cache.

\$\endgroup\$
  • \$\begingroup\$ Thanks a bunch for your inputs. The images example, the way I put it here, was more of a sample actually. The "key" also acts as the parameter using which the "data" is fetched, and also as the key in the cache. EmptyDataCallback and Status check can definitely be designed better. P.S great video/class :) \$\endgroup\$ – Dexter Sep 11 '15 at 6:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.