Java thread safety and caching techniques

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 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 list = new ArrayList();
for(int i = 0; i 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();

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 list = new ArrayList(); could be List list = new ArrayList();

• 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.

StackExchange Code Review Q#23610, answer score: 7