From 64040340c3ab0c8a73d940092143143daefeec76 Mon Sep 17 00:00:00 2001 From: Daniel Erat Date: Sun, 27 Mar 2011 14:19:48 -0700 Subject: [PATCH] android: Return copies of cached blobs instead of originals. This should fix http://code.google.com/p/camlistore/issues/detail?id=3, "Android client permits mutating the immutable cache". I'm also removing the call to URLConnection.guessContentTypeFromStream() in favor of just using URLConnection.guessContentTypeFromName(). I don't think that the former was ever successful, and it was hitting the disk from the UI thread. --- .../src/org/camlistore/BrowseActivity.java | 43 ++++++++--- .../src/org/camlistore/DownloadService.java | 74 +++++++++++++++---- clients/android/src/org/camlistore/Util.java | 12 +++ 3 files changed, 101 insertions(+), 28 deletions(-) diff --git a/clients/android/src/org/camlistore/BrowseActivity.java b/clients/android/src/org/camlistore/BrowseActivity.java index 8c67438e2..4764e4695 100644 --- a/clients/android/src/org/camlistore/BrowseActivity.java +++ b/clients/android/src/org/camlistore/BrowseActivity.java @@ -60,6 +60,13 @@ public class BrowseActivity extends ListActivity { // TODO: Remove this; it's pretty ugly. private HashMap mEntriesByContentBlobRef = new HashMap(); + // Map from the request code of an activity that we started to the file that we passed to it. + // We keep track of this so that we can delete the file when the activity is done. + private HashMap mOutstandingActivities = new HashMap(); + + // Next request code to allocate for |mOutstandingActivities|. + private int mNextActivityRequestCode = 1; + private enum EntryType { UNKNOWN("unknown"), FILE("file"), @@ -189,6 +196,24 @@ public class BrowseActivity extends ListActivity { unbindService(mConnection); } + @Override + protected void onActivityResult(int requestCode, int resultCode, Intent data) { + final File file = mOutstandingActivities.get(requestCode); + if (file == null) { + Log.e(TAG, "got unknown activity result with request code " + requestCode); + return; + } + mOutstandingActivities.remove(requestCode); + + Util.runAsync(new Runnable() { + @Override + public void run() { + Log.d(TAG, "deleting " + file.getPath()); + file.delete(); + } + }); + } + @Override protected void onListItemClick(ListView listView, View view, int position, long id) { Entry entry = mEntries.get(position); @@ -371,18 +396,9 @@ public class BrowseActivity extends ListActivity { return; } - // Try to guess the MIME type from the data itself first. + // Try to guess the mime type from the filename's extension. String mimeType = null; - try { - FileInputStream inputStream = new FileInputStream(file); - mimeType = URLConnection.guessContentTypeFromStream(inputStream); - inputStream.close(); - } catch (IOException e) { - Log.e(TAG, "got IO error while trying to guess mime type for " + file.getPath(), e); - } - - // If that didn't work, try to guess it from the filename. - if (mimeType == null && entry.getFilename() != null) + if (entry.getFilename() != null) mimeType = URLConnection.guessContentTypeFromName(entry.getFilename()); if (mimeType == null) mimeType = DEFAULT_MIME_TYPE; @@ -390,8 +406,11 @@ public class BrowseActivity extends ListActivity { Intent intent = new Intent(); intent.setAction(intent.ACTION_VIEW); intent.setDataAndType(Uri.fromFile(file), mimeType); + final int requestCode = mNextActivityRequestCode++; + if (mOutstandingActivities.put(requestCode, file) != null) + throw new RuntimeException("request code " + requestCode + " is already in use"); try { - startActivity(intent); + startActivityForResult(intent, requestCode); } catch (android.content.ActivityNotFoundException e) { Toast.makeText(BrowseActivity.this, "No activity found to display " + mimeType + ".", Toast.LENGTH_SHORT).show(); } diff --git a/clients/android/src/org/camlistore/DownloadService.java b/clients/android/src/org/camlistore/DownloadService.java index b3e543928..1b534b056 100644 --- a/clients/android/src/org/camlistore/DownloadService.java +++ b/clients/android/src/org/camlistore/DownloadService.java @@ -20,6 +20,7 @@ import android.app.Service; import android.content.Intent; import android.content.SharedPreferences; import android.os.Binder; +import android.os.ConditionVariable; import android.os.Handler; import android.os.IBinder; import android.util.Log; @@ -47,27 +48,38 @@ public class DownloadService extends Service { private static final int BUFFER_SIZE = 4096; private static final String USERNAME = "TODO-DUMMY-USER"; private static final String SEARCH_BLOBREF = "search"; + + // Name of directory on external storage where cached blobs are stored. private static final String BLOB_SUBDIR = "blobs"; + // Name of cache directory on external storage where we write copies of cached blobs + // to hand out to listeners. Cleared at startup. + private static final String BLOB_COPIES_SUBDIR = "blob_copies"; + private final IBinder mBinder = new LocalBinder(); private final Handler mHandler = new Handler(); // Effectively-final objects initialized in onCreate(). private SharedPreferences mSharedPrefs; private DownloadCache mCache; + // Directory on external storage where we make copies of blob files to hand to clients. + private File mBlobCopiesDir; // Protects members containing the state of current downloads. - private final ReentrantLock mDownloadLock = new ReentrantLock(); + private final ReentrantLock mLock = new ReentrantLock(); - // Blobs currently being downloaded. Protected by |mDownloadLock|. + // Blobs currently being downloaded. Protected by |mLock|. private final HashSet mInProgressBlobRefs = new HashSet(); - // Maps from blobrefs to callbacks for their contents. Protected by |mDownloadLock|. + // Maps from blobrefs to callbacks for their contents. Protected by |mLock|. private final HashMap> mByteArrayListenersByBlobRef = new HashMap>(); private final HashMap> mFileListenersByBlobRef = new HashMap>(); + // Closed until |mBlobCopiesDir| has been created and cleared. + private final ConditionVariable mBlobCopiesDirIsReady = new ConditionVariable(false); + // Callback for receiving a blob's contents as an in-memory array of bytes. interface ByteArrayListener { void onBlobDownloadSuccess(String blobRef, byte[] bytes); @@ -76,6 +88,8 @@ public class DownloadService extends Service { // Callback for receiving a blob's contents as a File. interface FileListener { + // The listener is responsible for deleting |file| once it's done using it. + // Abandoned files are cleared when the service starts. void onBlobDownloadSuccess(String blobRef, File file); void onBlobDownloadFailure(String data); } @@ -93,6 +107,20 @@ public class DownloadService extends Service { mSharedPrefs = getSharedPreferences(Preferences.NAME, 0); mCache = new DownloadCache(new File(getExternalFilesDir(null), BLOB_SUBDIR).getPath(), new Preferences(mSharedPrefs)); + + // Create |mBlobCopiesDir| and delete any stale files left over in it from a previous run. + mBlobCopiesDir = new File(getExternalCacheDir(), BLOB_COPIES_SUBDIR); + Util.runAsync(new Runnable() { + @Override + public void run() { + mBlobCopiesDir.mkdirs(); + for (File file : mBlobCopiesDir.listFiles()) { + Log.d(TAG, "deleting stale temp blob file " + file.getPath()); + file.delete(); + } + mBlobCopiesDirIsReady.open(); + } + }); } @Override @@ -131,7 +159,7 @@ public class DownloadService extends Service { // Get a list of byte array listeners waiting for |blobRef|. // If |insert| is true, the returned list can be used for adding new listeners. private ArrayList getByteArrayListenersForBlobRef(String blobRef, boolean insert) { - Util.assertLockIsHeld(mDownloadLock); + Util.assertLockIsHeld(mLock); ArrayList listeners = mByteArrayListenersByBlobRef.get(blobRef); if (listeners == null) { listeners = new ArrayList(); @@ -144,7 +172,7 @@ public class DownloadService extends Service { // Get a list of file listeners waiting for |blobRef|. // If |insert| is true, the returned list can be used for adding new listeners. private ArrayList getFileListenersForBlobRef(String blobRef, boolean insert) { - Util.assertLockIsHeld(mDownloadLock); + Util.assertLockIsHeld(mLock); ArrayList listeners = mFileListenersByBlobRef.get(blobRef); if (listeners == null) { listeners = new ArrayList(); @@ -176,7 +204,7 @@ public class DownloadService extends Service { @Override public void run() { - mDownloadLock.lock(); + mLock.lock(); try { if (mByteArrayListener != null) { getByteArrayListenersForBlobRef(mBlobRef, true).add(mByteArrayListener); @@ -193,7 +221,7 @@ public class DownloadService extends Service { return; mInProgressBlobRefs.add(mBlobRef); } finally { - mDownloadLock.unlock(); + mLock.unlock(); } mBlobFile = mCache.getFileForBlob(mBlobRef); @@ -249,9 +277,9 @@ public class DownloadService extends Service { return; } - mDownloadLock.lock(); + mLock.lock(); final boolean shouldDownloadToByteArray = !getByteArrayListenersForBlobRef(mBlobRef, false).isEmpty(); - mDownloadLock.unlock(); + mLock.unlock(); if (canBlobBeCached(mBlobRef)) { tempFile = mCache.getTempFileForDownload(mBlobRef, mSizeHintBytes); @@ -301,10 +329,10 @@ public class DownloadService extends Service { // handle the file listeners and any byte array listeners that were added in the meantime; this is just // an optimization so we don't block on disk if we only have byte array listeners. if (shouldDownloadToByteArray) { - mDownloadLock.lock(); + mLock.lock(); ArrayList byteArrayListeners = getByteArrayListenersForBlobRef(mBlobRef, false); mByteArrayListenersByBlobRef.remove(mBlobRef); - mDownloadLock.unlock(); + mLock.unlock(); mBlobBytes = ((ByteArrayOutputStream) outputStream).toByteArray(); sendBlobToByteArrayListeners(byteArrayListeners, mBlobBytes); @@ -362,16 +390,16 @@ public class DownloadService extends Service { // Removes |mBlobRef| from |mInProgressBlobRefs| and removes listeners from // |mByteArrayListenersByBlobRef| and |mFileListenersByBlobRef|. private void notifyListeners() { - mDownloadLock.lock(); + mLock.lock(); ArrayList byteArrayListeners = getByteArrayListenersForBlobRef(mBlobRef, false); mByteArrayListenersByBlobRef.remove(mBlobRef); ArrayList fileListeners = getFileListenersForBlobRef(mBlobRef, false); mFileListenersByBlobRef.remove(mBlobRef); mInProgressBlobRefs.remove(mBlobRef); - mDownloadLock.unlock(); + mLock.unlock(); // Make sure that we're not holding the lock; we're about to hit the disk. - Util.assertLockIsNotHeld(mDownloadLock); + Util.assertLockIsNotHeld(mLock); if (!byteArrayListeners.isEmpty()) { // If we don't have the data in memory already but it's on disk, read it. @@ -387,11 +415,25 @@ public class DownloadService extends Service { } for (final FileListener listener : fileListeners) { + File listenerFile = null; + if (mBlobFile != null) { + mBlobCopiesDirIsReady.block(); + // Make a copy of the file for each listener to ensure that they won't modify the cached blob. + try { + listenerFile = File.createTempFile(mBlobRef + "-", "", mBlobCopiesDir); + Log.d(TAG, "copying " + mBlobFile.getPath() + " to " + listenerFile.getPath()); + Util.copyFile(mBlobFile, listenerFile); + } catch (IOException e) { + Log.e(TAG, "got IO error while making copy of blob " + mBlobRef, e); + } + } + + final File finalListenerFile = listenerFile; mHandler.post(new Runnable() { @Override public void run() { - if (mBlobFile != null) { - listener.onBlobDownloadSuccess(mBlobRef, mBlobFile); + if (finalListenerFile != null) { + listener.onBlobDownloadSuccess(mBlobRef, finalListenerFile); } else { listener.onBlobDownloadFailure(mBlobRef); } diff --git a/clients/android/src/org/camlistore/Util.java b/clients/android/src/org/camlistore/Util.java index 59e4f271b..1a30b0a9a 100644 --- a/clients/android/src/org/camlistore/Util.java +++ b/clients/android/src/org/camlistore/Util.java @@ -18,8 +18,10 @@ package org.camlistore; import java.io.BufferedInputStream; import java.io.ByteArrayOutputStream; +import java.io.File; import java.io.FileDescriptor; import java.io.FileInputStream; +import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; import java.security.MessageDigest; @@ -52,6 +54,16 @@ public class Util { return outputStream.toByteArray(); } + public static void copyFile(File fromFile, File toFile) throws IOException { + FileInputStream inputStream = new FileInputStream(fromFile); + FileOutputStream outputStream = new FileOutputStream(toFile); + byte[] buffer = new byte[4096]; + for (int numRead; (numRead = inputStream.read(buffer)) != -1;) + outputStream.write(buffer, 0, numRead); + inputStream.close(); + outputStream.close(); + } + public static void runAsync(final Runnable r) { new AsyncTask() { @Override