Java thread safety and caching techniques

Question

This is my first Java multi-threading code. It is part of an Android application that is a client to a webpage that serves books. Each page of the book is in a separate PDF, and the Book class represents the entire book, and has functions to download and render individual pages. This class is supposed to make things running smoother by caching some pages ahead and behind the currently viewed page.

I am primarily interested in making sure this code is thread safe, but would be very happy to hear any thoughts and suggestions about improving it in any way.

import java.io.File;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.concurrent.locks.Condition;
import java.util.concurrent.locks.ReentrantLock;

import android.util.Log;

enum PageStatus {
    PENDING,
    DOWNLOADED,
    RENDERED
}

public class PageCacheManager {

    private static final String TAG = "PageCacheManager";

    private static final int DEFAULT_CACHE_SIZE_AHEAD = 5;
    private static final int DEFAULT_CACHE_SIZE_BEHIND = 3;

    private final Book mBook;
    private volatile PageCacheThread mCacheThread;
    private volatile int mLastPageRequested;
    private List <PageStatus> mPagesStatus;

    // For signaling that requested page is ready
    private ReentrantLock mLock = new ReentrantLock();
    private final Condition mPageReadyCondition = mLock.newCondition();

    public PageCacheManager(HebrewBook book, int page) {
        Log.i(TAG, "PageCacheManager created. BookID = " + book.getBookID());

        mBook = book;
        mLastPageRequested = page;

        ArrayList<PageStatus> list = new ArrayList<PageStatus>();
        for(int i = 0; i < mBook.getNumPages() + 1; i++) {
            list.add(PageStatus.PENDING);
        }
        mPagesStatus = Collections.synchronizedList(list);

        // Start the background thread
        mCacheThread = new PageCacheThread();
        mCacheThread.start();
    }

    public PageCacheManager(HebrewBook book) {
        this(book, 0);
    }

    public File getPage(int page) {
        Log.i(TAG, "getPage(): waiting for page " +  page);

        if(page < 1 || page > mBook.getNumPages()) {
            Log.e(TAG, "Requesting invalid page number");
            return null;
        }

        mLastPageRequested = page;

        // Make sure the background thread is running
        if(! mCacheThread.isAlive()) {
            mCacheThread = new PageCacheThread();
            mCacheThread.start();
        }

        // Wait for file to be rendered
        Log.i(TAG, "Waiting for page to be rendered");
        mLock.lock();
        try {
            while(mPagesStatus.get(page) != PageStatus.RENDERED) {
                mPageReadyCondition.await();
            }
        } catch (InterruptedException e) {
        } finally {
            mLock.unlock();
        }
        Log.i(TAG, "Recieved signal that page was rendered");


        // Find the file (asking it to be rendered when it already has, will just find it in the cache)
        File file = null;
        try {
            file = mBook.renderPage(page);
        } catch (Exception e) {
            Log.e(TAG, "Error: " + e.toString());
        }

        Log.i(TAG, "getPage, got page at " +  file.getAbsolutePath());

        return file;
    }

    private int getNextPageToDownload() {

        // Is there a page we have requested but hasn't been done yet?
        if((mLastPageRequested > 0) && (mPagesStatus.get(mLastPageRequested) == PageStatus.PENDING)) {
            return mLastPageRequested;
        }

        // Check ahead if any pages need to be cached

        int checkAhead = (mLastPageRequested + 1 + DEFAULT_CACHE_SIZE_AHEAD) <= mBook.getNumPages()
                ? mLastPageRequested + 1 + DEFAULT_CACHE_SIZE_AHEAD
                : mBook.getNumPages();

        for(int i = mLastPageRequested + 1; i < checkAhead; i++) {
            if(mPagesStatus.get(i) == PageStatus.PENDING) {
                return i;
            }
        }

        // Check behind if any pages need to be cached
        int checkBehind = (mLastPageRequested - 1 - DEFAULT_CACHE_SIZE_BEHIND) > 0
                ? mLastPageRequested - 1 - DEFAULT_CACHE_SIZE_BEHIND
                : 1;

        for(int i = mLastPageRequested; i > checkBehind; i--) {
            if(mPagesStatus.get(i) == PageStatus.PENDING) {
                return i;
            }
        }

        // Cache is up to date
        return 0;
    }

    class PageCacheThread extends Thread {

        @Override
        public void run() {

            Log.i(TAG, "PageCacheThread starting");

            int page = getNextPageToDownload();
            while(page != 0) {

                // Download and render the file

                try {
                    Log.i(TAG, "Downloading page " + page);
                    File file = mBook.getPage(page);
                    mPagesStatus.set(page, PageStatus.DOWNLOADED);
                    Log.i(TAG, "Download complete for page " + page + ". Now rendering");
                    Thread.yield();
                    mBook.renderPage(file);
                    Log.i(TAG, "Render complete for page " + page);
                    mPagesStatus.set(page, PageStatus.RENDERED);
                } catch (Exception e) {
                    // TODO: Better error handling
                    Log.e(TAG, "Error in PageCacheThread: " + e.toString());
                }

                // If we have rendered the requested page, notify other threads

                if(page == mLastPageRequested) {
                    Log.i(TAG, "Signalling that page is ready");
                    mLock.lock();
                    mPageReadyCondition.signalAll();
                    mLock.unlock();
                }

                page = getNextPageToDownload();
            }

            // TODO: investigate possible performance benefits of rendering in separate thread while downloading next page
        }

    }

}

Answer 1

Here are a few comments:

• you should not start a thread from the constructor - for example, the thread could see mPagesStatus as null and throw a NullPointerException. So you should provide an init method for example, that must be called by the client.
• I'm not a big fan of Hungarian notation (prefixing all instance variables with m) - my IDE already shows me what I need to know with a colour scheme.
• It is good practice to code to interface, for example: ArrayList<PageStatus> list = new ArrayList<PageStatus>(); could be List<PageStatus> list = new ArrayList<PageStatus>();
• mPagesStatus is only assigned once, I would make it final
• same thing for the lock: private final ReentrantLock mLock = new ReentrantLock();
• your PageCacheThread class is a Runnable really - I would make it so
• I would use an ExecutorService to manage the threads instead of manually starting them
• You don't really use the extra features of Condition, so I would use a simple wait/notifyAll mechanism and avoid the complexity of locks (for example, you don't always unlock your lock in a finally block, which can lead to deadlock)
• There seems to be an issue with your signalling code - mPageReadyCondition.signalAll(); is only called if page == mLastPageRequested - if two threads call getPage concurrently, you might fail to wake the conditions that are waiting. I would simply remove the if and signal every time a page is loaded
• For similar reasons, your getNextPageToDownload could miss an update if the getPage is called by two threads concurrently
• I would use a BlockingQueue mechanism for the pages to update, and put the page numbers in the queue from the getPage method and take from the queue in the Runnable.