diff --git a/CHANGELOG.md b/CHANGELOG.md index b103a9b8..9db056ef 100644 --- a/CHANGELOG.md +++ b/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: diff --git a/sdk-java/src/main/java/ly/count/sdk/java/Countly.java b/sdk-java/src/main/java/ly/count/sdk/java/Countly.java index 86486768..9f96eda8 100644 --- a/sdk-java/src/main/java/ly/count/sdk/java/Countly.java +++ b/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"; diff --git a/sdk-java/src/main/java/ly/count/sdk/java/internal/ModuleCrashes.java b/sdk-java/src/main/java/ly/count/sdk/java/internal/ModuleCrashes.java index 2c1365cc..45445ea9 100644 --- a/sdk-java/src/main/java/ly/count/sdk/java/internal/ModuleCrashes.java +++ b/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 logs = new ArrayList<>(); + //guarded by itself, as breadcrumbs are added on any thread and taken on the crash handler thread + protected final List 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 { diff --git a/sdk-java/src/main/java/ly/count/sdk/java/internal/SDKStorage.java b/sdk-java/src/main/java/ly/count/sdk/java/internal/SDKStorage.java index f4f2bf7e..19fe926d 100644 --- a/sdk-java/src/main/java/ly/count/sdk/java/internal/SDKStorage.java +++ b/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); diff --git a/sdk-java/src/main/java/ly/count/sdk/java/internal/Storage.java b/sdk-java/src/main/java/ly/count/sdk/java/internal/Storage.java index 255529b3..1add3b9b 100644 --- a/sdk-java/src/main/java/ly/count/sdk/java/internal/Storage.java +++ b/sdk-java/src/main/java/ly/count/sdk/java/internal/Storage.java @@ -228,8 +228,11 @@ public static Future 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; } diff --git a/sdk-java/src/test/java/ly/count/sdk/java/internal/CoreRobustnessTests.java b/sdk-java/src/test/java/ly/count/sdk/java/internal/CoreRobustnessTests.java index 860fb210..89a70338 100644 --- a/sdk-java/src/test/java/ly/count/sdk/java/internal/CoreRobustnessTests.java +++ b/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 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 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. * diff --git a/sdk-java/src/test/java/ly/count/sdk/java/internal/ModuleCrashesTests.java b/sdk-java/src/test/java/ly/count/sdk/java/internal/ModuleCrashesTests.java index 84d3f234..7b1e76c2 100644 --- a/sdk-java/src/test/java/ly/count/sdk/java/internal/ModuleCrashesTests.java +++ b/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 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 logs = crash.getLogs(); + if (logs != null) { + taken.addAndGet(logs.size()); + } + return null; + } + } + public static class TestCrashProcessor implements CrashProcessor { public TestCrashProcessor() {