Filestore thread safety fixes

sdk/src/androidTest/java/com/bugsnag/android/ErrorStoreTest.java

@@ -15,14 +15,15 @@
 import org.json.JSONException;
 import org.json.JSONObject;
 import org.junit.After;
-import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
 import java.io.File;
 import java.io.IOException;
 import java.io.StringWriter;
+import java.util.Collections;
+import java.util.List;
 
 @RunWith(AndroidJUnit4.class)
 @SmallTest
@@ -39,10 +40,9 @@ public class ErrorStoreTest {
      */
     @Before
     public void setUp() throws Exception {
-        Client client = new Client(InstrumentationRegistry.getContext(), "api-key");
-        config = client.config;
-        errorStore = client.errorStore;
-        Assert.assertNotNull(errorStore.storeDirectory);
+        config = new Configuration("api-key");
+        errorStore = new ErrorStore(config, InstrumentationRegistry.getContext());
+        assertNotNull(errorStore.storeDirectory);
         errorStorageDir = new File(errorStore.storeDirectory);
         FileUtils.clearFilesInDir(errorStorageDir);
     }
@@ -61,16 +61,21 @@ public void tearDown() throws Exception {
     public void testWrite() throws Exception {
         File[] files = errorStorageDir.listFiles();
         int baseline = files.length; // record baseline number of files
-
-        Error error = new Error.Builder(config, new RuntimeException(), null).build();
-        errorStore.write(error);
+        Error error = writeErrorToStore();
 
         files = errorStorageDir.listFiles();
         assertEquals(baseline + 1, files.length);
         File file = files[0];
         checkFileMatchesErrorReport(file, error);
     }
 
+    @NonNull
+    private Error writeErrorToStore() {
+        Error error = new Error.Builder(config, new RuntimeException(), null).build();
+        errorStore.write(error);
+        return error;
+    }
+
     @Test
     public void testIsLaunchCrashReport() throws Exception {
         String[] valid = {"1504255147933__30b7e350-dcd1-4032-969e-98d30be62bbc_startupcrash.json"};
@@ -123,6 +128,76 @@ public void isStartupCrash() throws Exception {
         assertFalse(errorStore.isStartupCrash(10000));
     }
 
+    @Test
+    public void testFindStoredFiles() {
+        assertEquals(0, errorStore.queuedFiles.size());
+        writeErrorToStore();
+        assertEquals(0, errorStore.queuedFiles.size());
+
+        List<File> storedFiles = errorStore.findStoredFiles();
+        assertEquals(1, storedFiles.size());
+        assertEquals(1, errorStore.queuedFiles.size());
+
+        writeErrorToStore();
+        writeErrorToStore();
+        storedFiles = errorStore.findStoredFiles();
+        assertEquals(2, storedFiles.size());
+        assertEquals(3, errorStore.queuedFiles.size());
+    }
+
+    @Test
+    public void testCancelQueuedFiles() {
+        assertEquals(0, errorStore.queuedFiles.size());
+        writeErrorToStore();
+        assertEquals(0, errorStore.queuedFiles.size());
+
+        List<File> storedFiles = errorStore.findStoredFiles();
+        assertEquals(1, storedFiles.size());
+        errorStore.cancelQueuedFiles(null);
+        assertEquals(1, errorStore.queuedFiles.size());
+
+        errorStore.cancelQueuedFiles(Collections.<File>emptyList());
+        assertEquals(1, errorStore.queuedFiles.size());
+
+        errorStore.cancelQueuedFiles(storedFiles);
+        assertEquals(0, errorStore.queuedFiles.size());
+    }
+
+    @Test
+    public void testDeleteQueuedFiles() {
+        assertEquals(0, errorStore.findStoredFiles().size());
+
+        writeErrorToStore();
+        List<File> storedFiles = errorStore.findStoredFiles();
+        assertEquals(1, storedFiles.size());
+
+        errorStore.deleteStoredFiles(null);
+        assertEquals(1, errorStore.queuedFiles.size());
+
+        errorStore.deleteStoredFiles(Collections.<File>emptyList());
+        assertEquals(1, errorStore.queuedFiles.size());
+
+
+        errorStore.deleteStoredFiles(storedFiles);
+        assertEquals(0, errorStore.findStoredFiles().size());
+        assertEquals(0, errorStore.queuedFiles.size());
+        assertEquals(0, new File(errorStore.storeDirectory).listFiles().length);
+    }
+
+    @Test
+    public void testFileQueueDuplication() {
+        writeErrorToStore();
+        List<File> ogFiles = errorStore.findStoredFiles();
+        assertEquals(1, ogFiles.size());
+
+        List<File> storedFiles = errorStore.findStoredFiles();
+        assertEquals(0, storedFiles.size());
+
+        errorStore.cancelQueuedFiles(ogFiles);
+        storedFiles = errorStore.findStoredFiles();
+        assertEquals(1, storedFiles.size());
+    }
+
     /**
      * Ensures that the file can be serialised back into a JSON report, and contains the same info
      * as the original

sdk/src/androidTest/java/com/bugsnag/android/SessionStoreTest.java

@@ -5,6 +5,7 @@
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 
+import android.content.Context;
 import android.support.test.InstrumentationRegistry;
 import android.support.test.filters.SmallTest;
 import android.support.test.runner.AndroidJUnit4;
@@ -29,8 +30,9 @@ public class SessionStoreTest {
      */
     @Before
     public void setUp() throws Exception {
-        Client client = new Client(InstrumentationRegistry.getContext(), "api-key");
-        SessionStore sessionStore = client.sessionStore;
+        Configuration config = new Configuration("api-key");
+        Context context = InstrumentationRegistry.getContext();
+        SessionStore sessionStore = new SessionStore(config, context);
         assertNotNull(sessionStore.storeDirectory);
         storageDir = new File(sessionStore.storeDirectory);
         FileUtils.clearFilesInDir(storageDir);

sdk/src/androidTest/java/com/bugsnag/android/SessionTrackingPayloadTest.java

@@ -36,8 +36,9 @@ public class SessionTrackingPayloadTest {
     @Before
     public void setUp() throws Exception {
         Context context = InstrumentationRegistry.getContext();
-        Client client = new Client(context, "api-key");
-        sessionStore = client.sessionStore;
+        Configuration config = new Configuration("api-key");
+        sessionStore = new SessionStore(config, context);
+
         Assert.assertNotNull(sessionStore.storeDirectory);
         storageDir = new File(sessionStore.storeDirectory);
         FileUtils.clearFilesInDir(storageDir);

sdk/src/main/java/com/bugsnag/android/ErrorStore.java

@@ -6,6 +6,7 @@
 import java.io.File;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
 import java.util.Locale;
@@ -126,18 +127,15 @@ private void flushErrorReport(File errorFile, ErrorReportApiClient errorReportAp
             errorReportApiClient.postReport(config.getEndpoint(), report,
                 config.getErrorApiHeaders());
 
+            deleteStoredFiles(Collections.singleton(errorFile));
             Logger.info("Deleting sent error file " + errorFile.getName());
-            if (!errorFile.delete()) {
-                errorFile.deleteOnExit();
-            }
         } catch (NetworkException exception) {
+            cancelQueuedFiles(Collections.singleton(errorFile));
             Logger.warn("Could not send previously saved error(s)"
                 + " to Bugsnag, will try again later", exception);
         } catch (Exception exception) {
+            deleteStoredFiles(Collections.singleton(errorFile));
             Logger.warn("Problem sending unsent error from disk", exception);
-            if (!errorFile.delete()) {
-                errorFile.deleteOnExit();
-            }
         }
     }
 

sdk/src/main/java/com/bugsnag/android/FileStore.java

@@ -9,8 +9,15 @@
 import java.io.Writer;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
 import java.util.Comparator;
+import java.util.HashSet;
 import java.util.List;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.ConcurrentSkipListSet;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
 
 abstract class FileStore<T extends JsonStream.Streamable> {
 
@@ -21,6 +28,9 @@ abstract class FileStore<T extends JsonStream.Streamable> {
     private final int maxStoreCount;
     private final Comparator<File> comparator;
 
+    final Lock lock = new ReentrantLock();
+    final Collection<File> queuedFiles = new ConcurrentSkipListSet<>();
+
     FileStore(@NonNull Configuration config, @NonNull Context appContext, String folder,
               int maxStoreCount, Comparator<File> comparator) {
         this.config = config;
@@ -54,20 +64,28 @@ String write(@NonNull T streamable) {
         File exceptionDir = new File(storeDirectory);
         if (exceptionDir.isDirectory()) {
             File[] files = exceptionDir.listFiles();
+
             if (files != null && files.length >= maxStoreCount) {
                 // Sort files then delete the first one (oldest timestamp)
                 Arrays.sort(files, comparator);
-                Logger.warn(String.format("Discarding oldest error as stored "
-                    + "error limit reached (%s)", files[0].getPath()));
-                if (!files[0].delete()) {
-                    files[0].deleteOnExit();
+
+                for (int k = 0; k < files.length && files.length >= maxStoreCount; k++) {
+                    File oldestFile = files[k];
+
+                    if (!queuedFiles.contains(oldestFile)) {
+                        Logger.warn(String.format("Discarding oldest error as stored "
+                            + "error limit reached (%s)", oldestFile.getPath()));
+                        deleteStoredFiles(Collections.singleton(oldestFile));
+                    }
                 }
             }
         }
 
         String filename = getFilename(streamable);
 
         Writer out = null;
+        lock.lock();
+
         try {
             out = new FileWriter(filename);
 
@@ -82,26 +100,67 @@ String write(@NonNull T streamable) {
                 filename), exception);
         } finally {
             IOUtils.closeQuietly(out);
+            lock.unlock();
         }
         return null;
     }
 
-    @NonNull abstract String getFilename(T streamable);
+    @NonNull
+    abstract String getFilename(T streamable);
 
     List<File> findStoredFiles() {
-        List<File> files = new ArrayList<>();
+        lock.lock();
+        try {
+            List<File> files = new ArrayList<>();
 
-        if (storeDirectory != null) {
-            File dir = new File(storeDirectory);
+            if (storeDirectory != null) {
+                File dir = new File(storeDirectory);
 
-            if (dir.exists() && dir.isDirectory()) {
-                File[] values = dir.listFiles();
+                if (dir.exists() && dir.isDirectory()) {
+                    File[] values = dir.listFiles();
 
-                if (values != null) {
-                    files.addAll(Arrays.asList(values));
+                    if (values != null) {
+                        for (File value : values) {
+                            if (value.isFile() && !queuedFiles.contains(value)) {
+                                files.add(value);
+                            }
+                        }
+                    }
                 }
             }
+            queuedFiles.addAll(files);
+            return files;
+        } finally {
+            lock.unlock();
         }
-        return files;
     }
+
+    void cancelQueuedFiles(Collection<File> files) {
+        lock.lock();
+        try {
+            if (files != null) {
+                queuedFiles.removeAll(files);
+            }
+        } finally {
+            lock.unlock();
+        }
+    }
+
+    void deleteStoredFiles(Collection<File> storedFiles) {
+        lock.lock();
+        try {
+            if (storedFiles != null) {
+                queuedFiles.removeAll(storedFiles);
+
+                for (File storedFile : storedFiles) {
+                    if (!storedFile.delete()) {
+                        storedFile.deleteOnExit();
+                    }
+                }
+            }
+        } finally {
+            lock.unlock();
+        }
+    }
+
 }

sdk/src/main/java/com/bugsnag/android/SessionTracker.java

@@ -171,12 +171,13 @@ void flushStoredSessions() {
                         final String endpoint = configuration.getSessionEndpoint();
                         apiClient.postSessionTrackingPayload(endpoint, payload,
                             configuration.getSessionApiHeaders());
-                        deleteStoredFiles(storedFiles);
+                        sessionStore.deleteStoredFiles(storedFiles);
                     } catch (NetworkException exception) { // store for later sending
+                        sessionStore.cancelQueuedFiles(storedFiles);
                         Logger.info("Failed to post stored session payload");
                     } catch (BadResponseException exception) { // drop bad data
                         Logger.warn("Invalid session tracking payload", exception);
-                        deleteStoredFiles(storedFiles);
+                        sessionStore.deleteStoredFiles(storedFiles);
                     }
                 }
             } finally {
@@ -185,12 +186,6 @@ void flushStoredSessions() {
         }
     }
 
-    private void deleteStoredFiles(Collection<File> storedFiles) {
-        for (File storedFile : storedFiles) {
-            storedFile.delete();
-        }
-    }
-
     @Override
     public void onActivityCreated(@NonNull Activity activity, Bundle savedInstanceState) {
         leaveLifecycleBreadcrumb(getActivityName(activity), "onCreate()");