Breadcrumbs during a crash, the logger on a refused init, missing storage file errors

CHANGELOG.md

@@ -42,6 +42,8 @@
 
 * Mitigated an issue where requests, sessions or crashes created close together could get the same storage ID, so some of them were lost.
 * Mitigated an issue where verbose logs showed the values of custom network request headers. Only the number of headers is logged now.
+* Mitigated an issue where a fresh initialization logged an error for storage files that did not exist yet.
+* Mitigated an issue where a crash breadcrumb added while a crash was being recorded on another thread could be lost.
 
 ## 26.8.0
 * Added support for the Content feature, accessible through the "Countly.instance().content()" interface:

sdk-java/src/main/java/ly/count/sdk/java/Countly.java

@@ -84,7 +84,6 @@ public void init(final Config config) {
         }
 
         Log L = new Log(internalConfig.loggingLevel, internalConfig.logListener);
-        internalConfig.setLogger(L);
 
         if (directory == null) {
             L.e("[Countly] File cannot be null");
@@ -104,6 +103,7 @@ public void init(final Config config) {
             L.e("[Countly] Countly shouldn't be initialized twice. Please either use Countly.isInitialized() to check status or call Countly.stop() before second Countly.init(). Calling 'init' the second time will now do nothing");
             return;
         }
+        internalConfig.setLogger(L);
 
         if (internalConfig.enableBackendMode) {
             internalConfig.sdkName = "java-native-backend";

sdk-java/src/main/java/ly/count/sdk/java/internal/ModuleCrashes.java

@@ -27,7 +27,8 @@ public class ModuleCrashes extends ModuleBase {
     //written under crashHandlerLock
     private volatile boolean stopped = false;
     protected CrashProcessor crashProcessor = null;
-    protected List<String> logs = new ArrayList<>();
+    //guarded by itself, as breadcrumbs are added on any thread and taken on the crash handler thread
+    protected final List<String> logs = new ArrayList<>();
 
     Crashes crashInterface;
 
@@ -56,7 +57,9 @@ public void stop(InternalConfig config, boolean clear) {
         } catch (Throwable t) {
             L.e("[ModuleCrash] Exception while stopping crash reporting" + t);
         }
-        logs.clear();
+        synchronized (logs) {
+            logs.clear();
+        }
         crashInterface = null;
     }
 
@@ -215,9 +218,11 @@ public CrashImpl onCrash(InternalConfig config, CrashImpl crash) {
             L.w("[ModuleCrash] onCrash, While recording an exception 'App version' was either null or empty");
         }
 
-        if (!logs.isEmpty()) {
-            crash.setLogs(logs.toArray(new String[0]));
-            logs.clear();
+        synchronized (logs) {
+            if (!logs.isEmpty()) {
+                crash.setLogs(logs.toArray(new String[0]));
+                logs.clear();
+            }
         }
 
         L.i("[ModuleCrash] onCrash: " + crash.getJSON());
@@ -255,13 +260,15 @@ protected void addBreadcrumbInternal(String record) {
         ConfigurationProvider configProvider = internalConfig.getConfigurationProvider();
         String breadcrumb = UtilsInternalLimits.truncateValue(record, configProvider.getMaxValueSize(), L, "[ModuleCrash] addBreadcrumbInternal");
 
-        //a loop, as the limit can shrink while breadcrumbs are kept
         int maxBreadcrumbCount = configProvider.getMaxBreadcrumbCount();
-        while (!logs.isEmpty() && logs.size() >= maxBreadcrumbCount) {
-            logs.remove(0);
-        }
+        synchronized (logs) {
+            //a loop, as the limit can shrink while breadcrumbs are kept
+            while (!logs.isEmpty() && logs.size() >= maxBreadcrumbCount) {
+                logs.remove(0);
+            }
 
-        logs.add(breadcrumb);
+            logs.add(breadcrumb);
+        }
     }
 
     public class Crashes {

sdk-java/src/main/java/ly/count/sdk/java/internal/SDKStorage.java

@@ -171,6 +171,11 @@ private FileOutputStream openFileAsOutputStream(ly.count.sdk.java.internal.Inter
     }
 
     public byte[] storableReadBytes(InternalConfig config, String filename) {
+        //every storable starts without a file, so a missing one is no error
+        if (!new File(createFileFullPath(config, filename)).exists()) {
+            return null;
+        }
+
         ByteArrayOutputStream buffer = null;
         FileInputStream stream = null;
 
@@ -189,6 +194,7 @@ public byte[] storableReadBytes(InternalConfig config, String filename) {
 
             return buffer.toByteArray();
         } catch (FileNotFoundException e) {
+            L.e("[SDKStorage] Error while opening file " + filename + " " + e);
             return null;
         } catch (IOException e) {
             L.e("[SDKStorage] Error while reading file " + filename + " " + e);

sdk-java/src/main/java/ly/count/sdk/java/internal/Storage.java

@@ -228,8 +228,11 @@ public static <T extends Storable> Future<T> readAsync(final InternalConfig conf
             public T call() throws Exception {
                 Boolean done = config.sdk.sdkStorage.storableRead(config, storable);
                 T ret = null;
-                if (done == null || !done) {
-                    config.getLogger().e("[Storage] No data for file " + name(storable));
+                if (done == null) {
+                    //every storable starts without a file, and one that could not be read was logged as an error already
+                    config.getLogger().d("[Storage] No data for file " + name(storable));
+                } else if (!done) {
+                    config.getLogger().e("[Storage] Could not restore the data of file " + name(storable));
                 } else {
                     ret = storable;
                 }

sdk-java/src/test/java/ly/count/sdk/java/internal/CoreRobustnessTests.java

@@ -1,7 +1,9 @@
 package ly.count.sdk.java.internal;
 
 import java.io.File;
+import java.util.List;
 import java.util.Map;
+import java.util.concurrent.CopyOnWriteArrayList;
 import ly.count.sdk.java.Config;
 import ly.count.sdk.java.Countly;
 import org.junit.After;
@@ -166,6 +168,62 @@ public void aStorableThatCannotSerialise_leavesNoFileBehind() {
         Assert.assertEquals("no file should have been created, found: " + litter.length, 0, litter.length);
     }
 
+    /**
+     * A fresh init reads storage files that do not exist yet, which is the normal first run state:
+     * each one is logged at debug level, and no error is logged for any of them.
+     */
+    @Test
+    public void freshInit_logsTheStorageFilesThatDoNotExistYet_atDebugLevel() {
+        List<String> missingFileLines = new CopyOnWriteArrayList<>();
+        Countly.instance().init(TestUtils.getConfigEvents(2).setLoggingLevel(Config.LoggingLevel.VERBOSE)
+            .setLogListener((message, level) -> {
+                if (message.contains("No data for file")) {
+                    missingFileLines.add(level + " " + message);
+                }
+            }));
+
+        Assert.assertFalse(missingFileLines.isEmpty());
+        for (String line : missingFileLines) {
+            Assert.assertTrue(line, line.startsWith(Config.LoggingLevel.DEBUG + " "));
+        }
+    }
+
+    /**
+     * A storage file that exists but cannot be opened, unlike a missing one, is logged as an error.
+     */
+    @Test
+    public void init_withAStorageFileThatCannotBeOpened_logsAnError() {
+        Assert.assertTrue(new File(TestUtils.getTestSDirectory(), "[CLY]_timedEvent_0").mkdir());
+        List<String> errors = new CopyOnWriteArrayList<>();
+        Countly.instance().init(TestUtils.getConfigEvents(2).setLoggingLevel(Config.LoggingLevel.VERBOSE)
+            .setLogListener((message, level) -> {
+                if (level == Config.LoggingLevel.ERROR && message.contains("timedEvent_0")) {
+                    errors.add(message);
+                }
+            }));
+
+        Assert.assertEquals(errors.toString(), 1, errors.size());
+        Assert.assertTrue(errors.get(0), errors.get(0).contains("Error while opening file"));
+    }
+
+    /**
+     * A second init is refused before it changes the running SDK, so given the same configuration
+     * object again it leaves the logger the running SDK and its configuration log through in place.
+     */
+    @Test
+    public void secondInit_withTheSameConfigObject_keepsTheRunningLogger() {
+        InternalConfig config = new InternalConfig(TestUtils.getConfigEvents(2));
+        Countly.instance().init(config);
+        Log running = SDKCore.instance.L;
+        Assert.assertSame(running, config.getLogger());
+
+        Countly.instance().init(config);
+
+        Assert.assertTrue(Countly.isInitialized());
+        Assert.assertSame(running, SDKCore.instance.L);
+        Assert.assertSame(running, config.getLogger());
+    }
+
     /**
      * The queued crash request, or null when none was queued.
      *

sdk-java/src/test/java/ly/count/sdk/java/internal/ModuleCrashesTests.java

@@ -2,8 +2,12 @@
 
 import java.io.PrintWriter;
 import java.io.StringWriter;
+import java.util.List;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.BiConsumer;
 import ly.count.sdk.java.Config;
 import ly.count.sdk.java.Countly;
@@ -146,6 +150,45 @@ public void addCrashBreadcrumb() {
         validateCrashInRQ(testThrowable, false, null, null, 1);
     }
 
+    /**
+     * "addCrashBreadcrumb" while the uncaught exception handler records crashes
+     * Validating that breadcrumbs added through the interface while crashes are recorded on another
+     * thread, as the uncaught exception handler does, are each taken by one crash or kept for the
+     * next, none of them lost, and that no call throws
+     */
+    @Test
+    public void addCrashBreadcrumb_whileAnotherThreadRecordsCrashes_losesNone() throws InterruptedException {
+        BreadcrumbCountingProcessor.taken.set(0);
+        int breadcrumbs = 20_000;
+        Countly.instance().init(TestUtils.getBaseConfig().enableFeatures(Config.Feature.CrashReporting)
+            .setMaxBreadcrumbCount(breadcrumbs).setCrashProcessorClass(BreadcrumbCountingProcessor.class));
+        ModuleCrashes module = SDKCore.instance.module(ModuleCrashes.class);
+        List<Throwable> failures = new CopyOnWriteArrayList<>();
+        AtomicBoolean adding = new AtomicBoolean(true);
+
+        Thread crashes = new Thread(() -> {
+            try {
+                while (adding.get()) {
+                    module.recordExceptionInternal(new IllegalStateException("crash"), false, null, null);
+                }
+            } catch (Throwable t) {
+                failures.add(t);
+            }
+        });
+        crashes.start();
+        try {
+            for (int i = 0; i < breadcrumbs; i++) {
+                Countly.instance().crashes().addCrashBreadcrumb("breadcrumb" + i);
+            }
+        } finally {
+            adding.set(false);
+            crashes.join(30_000);
+        }
+
+        Assert.assertTrue("failed: " + failures, failures.isEmpty());
+        Assert.assertEquals(breadcrumbs, BreadcrumbCountingProcessor.taken.get() + module.logs.size());
+    }
+
     /**
      * "addCrashBreadcrumb" with default "maxBreadcrumbCount"
      * Validating that handled exception is recorded correctly to request queue and has breadcrumbs,
@@ -270,6 +313,25 @@ private void recordExceptionWithSegment_base(boolean fatal, BiConsumer<Throwable
         validateCrashInRQ(testThrowable, fatal, new JSONObject(customSegment), null, 0);
     }
 
+    /**
+     * Counts the breadcrumbs of every crash it is given and drops the crash.
+     */
+    public static class BreadcrumbCountingProcessor implements CrashProcessor {
+        static final AtomicInteger taken = new AtomicInteger();
+
+        public BreadcrumbCountingProcessor() {
+        }
+
+        @Override
+        public Crash process(Crash crash) {
+            List<String> logs = crash.getLogs();
+            if (logs != null) {
+                taken.addAndGet(logs.size());
+            }
+            return null;
+        }
+    }
+
     public static class TestCrashProcessor implements CrashProcessor {
 
         public TestCrashProcessor() {