Merge pull request #48 from backtrace-labs/bugfix-breadcrumb-trimming

Fix issue rolling over Breadcrumb file

Author: vlussenburg

backtrace-library/src/androidTest/java/backtraceio/library/breadcrumbs/BacktraceBreadcrumbsTest.java

@@ -19,6 +19,8 @@
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Paths;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.LinkedHashMap;
@@ -308,8 +310,8 @@ public void testQueueFileShouldNotRollover() {
 
     @Test
     public void testQueueFileRollover() {
+        // Should reach the max of 64kB around breadcrumb 925
         final int numIterations = 1000;
-        final int firstBreadcrumbIndexAfterRollover = 925;
         // Account for mandatory configuration breadcrumb
         backtraceBreadcrumbs.setCurrentBreadcrumbId(1);
 
@@ -322,9 +324,12 @@ public void testQueueFileRollover() {
                 backtraceBreadcrumbs.addBreadcrumb("I am a breadcrumb", attributes);
             }
 
-            List<String> breadcrumbLogFileData = readBreadcrumbLogFile();
+            long breadcrumbsFileSize = Files.size(Paths.get(backtraceBreadcrumbs.getBreadcrumbLogPath()));
+            assertTrue(String.format("Size of breadcrumbs file (%s) not close enough to a full breadcrumb file (%s)", breadcrumbsFileSize, 64 * 1024),
+                    breadcrumbsFileSize > 63 * 1024);
 
             // We should have rolled over the configuration breadcrumb, consider all breadcrumbs here
+            List<String> breadcrumbLogFileData = readBreadcrumbLogFile();
             for (int i = 0; i < breadcrumbLogFileData.size(); i++) {
                 JSONObject parsedBreadcrumb = new JSONObject(breadcrumbLogFileData.get(i));
                 assertEquals("I am a breadcrumb", parsedBreadcrumb.get("message"));
@@ -333,8 +338,9 @@ public void testQueueFileRollover() {
                 assertEquals("info", parsedBreadcrumb.get("level"));
                 // Timestamp should be convertible to a long
                 assertTrue(parsedBreadcrumb.get("timestamp") instanceof Long);
-                assertTrue(((int) parsedBreadcrumb.get("id")) > firstBreadcrumbIndexAfterRollover);
-                assertTrue((int) parsedBreadcrumb.get("id") <= numIterations);
+                final int id = (int) parsedBreadcrumb.get("id");
+                assertTrue(String.format("Breadcrumb ID %s was higher than the expected numIterations %s",
+                        id, numIterations), id <= numIterations);
             }
 
         } catch (Exception ex) {

backtrace-library/src/main/java/backtraceio/library/breadcrumbs/BacktraceQueueFileHelper.java

@@ -37,8 +37,7 @@ public BacktraceQueueFileHelper(String breadcrumbLogDirectory, int maxQueueFileS
 
         // QueueFile pre-allocates a file of a certain size and fills with empty data,
         // so normal File operations will not give us an accurate count of the bytes
-        // in the file.
-        // Therefore we expose the private method QueueFile::usedBytes
+        // in the file. Therefore we expose the private method QueueFile::usedBytes.
         usedBytes = QueueFile.class.getDeclaredMethod("usedBytes");
         usedBytes.setAccessible(true);
 
@@ -51,7 +50,6 @@ public BacktraceQueueFileHelper(String breadcrumbLogDirectory, int maxQueueFileS
 
     public boolean add(byte[] bytes) {
         try {
-            int usedBytes = (int) this.usedBytes.invoke(breadcrumbStore);
             int breadcrumbLength = bytes.length;
 
             if (breadcrumbLength > 4096) {
@@ -61,7 +59,13 @@ public boolean add(byte[] bytes) {
 
             // We clear the space we need from the QueueFile first to prevent
             // the QueueFile from expanding to accommodate the new breadcrumb
-            while (!breadcrumbStore.isEmpty() && (usedBytes + breadcrumbLength) > maxQueueFileSizeBytes) {
+            // Note!
+            // In the 1.2.3 version we use, the usedBytes is int, not long, and the 
+            // implementation is synchronized thus safe for multithreaded access.
+            // see https://github.com/square/tape/blob/tape-parent-1.2.3/tape/src/main/java/com/squareup/tape/QueueFile.java
+            for (int usedBytes = (int) this.usedBytes.invoke(breadcrumbStore);
+                 !breadcrumbStore.isEmpty() && (usedBytes + breadcrumbLength) > maxQueueFileSizeBytes;
+                 usedBytes = (int) this.usedBytes.invoke(breadcrumbStore)) {
                 breadcrumbStore.remove();
             }