Java线程安全和缓存技术

Question

这是我的第一个Java多线程代码。它是Android应用程序的一部分，它是为图书服务的网页的客户端。这本书的每一页都是在一个单独的PDF中，book类代表整个书，并具有下载和呈现单个页面的功能。这个类应该通过缓存当前查看的页面后面和前面的一些页面来使运行更加顺畅。

我主要感兴趣的是确保这段代码是线程安全的，但我很乐意听到关于以任何方式改进它的任何想法和建议。

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

以下是一些评论：

• 不应从构造函数启动线程。 --例如，线程可以将mPagesStatus视为null并抛出一个NullPointerException。因此，您应该提供一个init方法，例如，客户端必须调用该方法。
• 我不太喜欢匈牙利符号(用m作为所有实例变量的前缀)--我的IDE已经向我展示了我需要了解的配色方案。
• 编写接口代码是很好的实践，例如：ArrayList<PageStatus> list = new ArrayList<PageStatus>();可以是List<PageStatus> list = new ArrayList<PageStatus>();
• mPagesStatus只分配一次，我会把它变成final
• 同样的锁：private final ReentrantLock mLock = new ReentrantLock();
• 您的PageCacheThread类实际上是一个Runnable -我会这样做的。
• 我将使用ExecutorService来管理线程，而不是手动启动它们。
• 您并不真正使用Condition的额外特性，所以我将使用简单的等待/通知所有机制，并避免锁的复杂性(例如，您并不总是在一个最终块中解锁，这会导致死锁)
• 您的信令代码似乎存在一个问题--只有当mPageReadyCondition.signalAll();被调用时才调用page == mLastPageRequested --如果两个线程同时调用getPage，您可能无法唤醒等待的条件。每次加载页面时，我都会删除if并发出信号。
• 出于类似的原因，如果由两个线程同时调用getNextPageToDownload，则getPage可能会错过更新。
• 我将使用BlockingQueue机制来更新页面，并将页面编号从getPage方法放到队列中，将take放在Runnable中的队列中。