diff --git a/CHANGELOG.md b/CHANGELOG.md index ead93ea6468..3025eba706e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ - Add current activity name to app context ([#2999](https://github.com/getsentry/sentry-java/pull/2999)) +### Fixes + +- Reduce main thread work on init ([#3036](https://github.com/getsentry/sentry-java/pull/3036)) + ## 6.33.1 ### Fixes diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/EnvelopeFileObserverIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/EnvelopeFileObserverIntegration.java index 140b9449822..85f2cc048fe 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/EnvelopeFileObserverIntegration.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/EnvelopeFileObserverIntegration.java @@ -37,28 +37,42 @@ public final void register(final @NotNull IHub hub, final @NotNull SentryOptions logger.log( SentryLevel.DEBUG, "Registering EnvelopeFileObserverIntegration for path: %s", path); - final OutboxSender outboxSender = - new OutboxSender( - hub, - options.getEnvelopeReader(), - options.getSerializer(), - logger, - options.getFlushTimeoutMillis()); - - observer = - new EnvelopeFileObserver(path, outboxSender, logger, options.getFlushTimeoutMillis()); try { - observer.startWatching(); - logger.log(SentryLevel.DEBUG, "EnvelopeFileObserverIntegration installed."); + options.getExecutorService().submit(() -> startOutboxSender(hub, options, path)); } catch (Throwable e) { - // it could throw eg NoSuchFileException or NullPointerException - options - .getLogger() - .log(SentryLevel.ERROR, "Failed to initialize EnvelopeFileObserverIntegration.", e); + logger.log( + SentryLevel.DEBUG, + "Failed to start EnvelopeFileObserverIntegration on executor thread. Starting on the calling thread.", + e); + startOutboxSender(hub, options, path); } } } + private void startOutboxSender( + final @NotNull IHub hub, final @NotNull SentryOptions options, final @NotNull String path) { + final OutboxSender outboxSender = + new OutboxSender( + hub, + options.getEnvelopeReader(), + options.getSerializer(), + options.getLogger(), + options.getFlushTimeoutMillis()); + + observer = + new EnvelopeFileObserver( + path, outboxSender, options.getLogger(), options.getFlushTimeoutMillis()); + try { + observer.startWatching(); + options.getLogger().log(SentryLevel.DEBUG, "EnvelopeFileObserverIntegration installed."); + } catch (Throwable e) { + // it could throw eg NoSuchFileException or NullPointerException + options + .getLogger() + .log(SentryLevel.ERROR, "Failed to initialize EnvelopeFileObserverIntegration.", e); + } + } + @Override public void close() { if (observer != null) { diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/SendCachedEnvelopeIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/SendCachedEnvelopeIntegration.java index e0d08325b45..e201d8535e6 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/SendCachedEnvelopeIntegration.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/SendCachedEnvelopeIntegration.java @@ -40,20 +40,21 @@ public void register(@NotNull IHub hub, @NotNull SentryOptions options) { return; } - final SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForget sender = - factory.create(hub, androidOptions); - - if (sender == null) { - androidOptions.getLogger().log(SentryLevel.ERROR, "SendFireAndForget factory is null."); - return; - } - try { Future future = androidOptions .getExecutorService() .submit( () -> { + final SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForget sender = + factory.create(hub, androidOptions); + + if (sender == null) { + androidOptions + .getLogger() + .log(SentryLevel.ERROR, "SendFireAndForget factory is null."); + return; + } try { sender.send(); } catch (Throwable e) { diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/EnvelopeFileObserverIntegrationTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/EnvelopeFileObserverIntegrationTest.kt index 2fcc6415e4d..6bdafac731b 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/EnvelopeFileObserverIntegrationTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/EnvelopeFileObserverIntegrationTest.kt @@ -2,10 +2,17 @@ package io.sentry.android.core import androidx.test.ext.junit.runners.AndroidJUnit4 import io.sentry.Hub +import io.sentry.IHub +import io.sentry.ILogger +import io.sentry.SentryLevel import io.sentry.SentryOptions +import io.sentry.test.ImmediateExecutorService import org.junit.runner.RunWith +import org.mockito.kotlin.eq import org.mockito.kotlin.mock +import org.mockito.kotlin.never import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever import java.io.File import java.nio.file.Files import kotlin.test.AfterTest @@ -15,6 +22,25 @@ import kotlin.test.assertEquals @RunWith(AndroidJUnit4::class) class EnvelopeFileObserverIntegrationTest { + inner class Fixture { + val hub: IHub = mock() + private lateinit var options: SentryAndroidOptions + val logger = mock() + + fun getSut(optionConfiguration: (SentryAndroidOptions) -> Unit = {}): EnvelopeFileObserverIntegration { + options = SentryAndroidOptions() + options.setLogger(logger) + options.isDebug = true + optionConfiguration(options) + whenever(hub.options).thenReturn(options) + + return object : EnvelopeFileObserverIntegration() { + override fun getPath(options: SentryOptions): String? = file.absolutePath + } + } + } + + private val fixture = Fixture() private lateinit var file: File @@ -51,4 +77,32 @@ class EnvelopeFileObserverIntegrationTest { hub.close() verify(integrationMock).close() } + + @Test + fun `register with fake executor service does not install integration`() { + val integration = fixture.getSut { + it.executorService = mock() + } + integration.register(fixture.hub, fixture.hub.options) + verify(fixture.logger).log( + eq(SentryLevel.DEBUG), + eq("Registering EnvelopeFileObserverIntegration for path: %s"), + eq(file.absolutePath) + ) + verify(fixture.logger, never()).log(eq(SentryLevel.DEBUG), eq("EnvelopeFileObserverIntegration installed.")) + } + + @Test + fun `register integration on the background via executor service`() { + val integration = fixture.getSut { + it.executorService = ImmediateExecutorService() + } + integration.register(fixture.hub, fixture.hub.options) + verify(fixture.logger).log( + eq(SentryLevel.DEBUG), + eq("Registering EnvelopeFileObserverIntegration for path: %s"), + eq(file.absolutePath) + ) + verify(fixture.logger).log(eq(SentryLevel.DEBUG), eq("EnvelopeFileObserverIntegration installed.")) + } } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/SendCachedEnvelopeIntegrationTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/SendCachedEnvelopeIntegrationTest.kt index 36094f78ebb..1fa08986b41 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/SendCachedEnvelopeIntegrationTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/SendCachedEnvelopeIntegrationTest.kt @@ -2,9 +2,12 @@ package io.sentry.android.core import io.sentry.IHub import io.sentry.ILogger +import io.sentry.ISentryExecutorService import io.sentry.SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForget import io.sentry.SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForgetFactory import io.sentry.SentryLevel.DEBUG +import io.sentry.SentryOptions +import io.sentry.test.ImmediateExecutorService import io.sentry.util.LazyEvaluator import org.awaitility.kotlin.await import org.mockito.kotlin.any @@ -32,11 +35,13 @@ class SendCachedEnvelopeIntegrationTest { hasStartupCrashMarker: Boolean = false, hasSender: Boolean = true, delaySend: Long = 0L, - taskFails: Boolean = false + taskFails: Boolean = false, + mockExecutorService: ISentryExecutorService? = null ): SendCachedEnvelopeIntegration { options.cacheDirPath = cacheDirPath options.setLogger(logger) options.isDebug = true + options.executorService = mockExecutorService ?: SentryOptions().executorService whenever(sender.send()).then { Thread.sleep(delaySend) @@ -71,7 +76,7 @@ class SendCachedEnvelopeIntegrationTest { @Test fun `when factory returns null, does nothing`() { - val sut = fixture.getSut(hasSender = false) + val sut = fixture.getSut(hasSender = false, mockExecutorService = ImmediateExecutorService()) sut.register(fixture.hub, fixture.options) @@ -81,7 +86,7 @@ class SendCachedEnvelopeIntegrationTest { @Test fun `when has factory and cacheDirPath set, submits task into queue`() { - val sut = fixture.getSut() + val sut = fixture.getSut(mockExecutorService = ImmediateExecutorService()) sut.register(fixture.hub, fixture.options) @@ -89,6 +94,15 @@ class SendCachedEnvelopeIntegrationTest { verify(fixture.sender).send() } + @Test + fun `when executorService is fake, does nothing`() { + val sut = fixture.getSut(mockExecutorService = mock()) + sut.register(fixture.hub, fixture.options) + + verify(fixture.factory, never()).create(any(), any()) + verify(fixture.sender, never()).send() + } + @Test fun `when has startup crash marker, awaits the task on the calling thread`() { val sut = fixture.getSut(hasStartupCrashMarker = true) diff --git a/sentry/src/main/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegration.java b/sentry/src/main/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegration.java index 01f9b960183..16826b2c50e 100644 --- a/sentry/src/main/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegration.java +++ b/sentry/src/main/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegration.java @@ -64,18 +64,17 @@ public final void register(final @NotNull IHub hub, final @NotNull SentryOptions return; } - final SendFireAndForget sender = factory.create(hub, options); - - if (sender == null) { - options.getLogger().log(SentryLevel.ERROR, "SendFireAndForget factory is null."); - return; - } - try { options .getExecutorService() .submit( () -> { + final SendFireAndForget sender = factory.create(hub, options); + + if (sender == null) { + options.getLogger().log(SentryLevel.ERROR, "SendFireAndForget factory is null."); + return; + } try { sender.send(); } catch (Throwable e) { diff --git a/sentry/src/main/java/io/sentry/Sentry.java b/sentry/src/main/java/io/sentry/Sentry.java index 71b588d55b9..fbb9997b948 100644 --- a/sentry/src/main/java/io/sentry/Sentry.java +++ b/sentry/src/main/java/io/sentry/Sentry.java @@ -335,18 +335,21 @@ private static boolean initConfigurations(final @NotNull SentryOptions options) final File profilingTracesDir = new File(profilingTracesDirPath); profilingTracesDir.mkdirs(); - final File[] oldTracesDirContent = profilingTracesDir.listFiles(); + final long timestamp = System.currentTimeMillis(); try { options .getExecutorService() .submit( () -> { + final File[] oldTracesDirContent = profilingTracesDir.listFiles(); if (oldTracesDirContent == null) return; // Method trace files are normally deleted at the end of traces, but if that fails // for some reason we try to clear any old files here. for (File f : oldTracesDirContent) { - FileUtils.deleteRecursively(f); + if (f.lastModified() < timestamp) { + FileUtils.deleteRecursively(f); + } } }); } catch (RejectedExecutionException e) { diff --git a/sentry/src/test/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegrationTest.kt b/sentry/src/test/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegrationTest.kt index a1c06d31dba..12d78d46dda 100644 --- a/sentry/src/test/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegrationTest.kt +++ b/sentry/src/test/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegrationTest.kt @@ -1,9 +1,11 @@ package io.sentry import io.sentry.protocol.SdkVersion +import io.sentry.test.ImmediateExecutorService import org.mockito.kotlin.any import org.mockito.kotlin.eq import org.mockito.kotlin.mock +import org.mockito.kotlin.never import org.mockito.kotlin.verify import org.mockito.kotlin.verifyNoMoreInteractions import org.mockito.kotlin.whenever @@ -65,6 +67,7 @@ class SendCachedEnvelopeFireAndForgetIntegrationTest { fun `when Factory returns null, register logs and exit`() { val sut = SendCachedEnvelopeFireAndForgetIntegration(CustomFactory()) fixture.options.cacheDirPath = "abc" + fixture.options.executorService = ImmediateExecutorService() sut.register(fixture.hub, fixture.options) verify(fixture.logger).log(eq(SentryLevel.ERROR), eq("SendFireAndForget factory is null.")) verifyNoMoreInteractions(fixture.hub) @@ -92,6 +95,24 @@ class SendCachedEnvelopeFireAndForgetIntegrationTest { verify(fixture.logger).log(eq(SentryLevel.ERROR), eq("Failed to call the executor. Cached events will not be sent. Did you call Sentry.close()?"), any()) } + @Test + fun `register runs on executor service`() { + fixture.options.executorService = ImmediateExecutorService() + fixture.options.cacheDirPath = "cache" + val sut = fixture.getSut() + sut.register(fixture.hub, fixture.options) + verify(fixture.callback).create(eq(fixture.hub), eq(fixture.options)) + } + + @Test + fun `does not register on fake executor service`() { + fixture.options.executorService = mock() + fixture.options.cacheDirPath = "cache" + val sut = fixture.getSut() + sut.register(fixture.hub, fixture.options) + verify(fixture.callback, never()).create(any(), any()) + } + private class CustomFactory : SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForgetFactory { override fun create(hub: IHub, options: SentryOptions): SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForget? { return null diff --git a/sentry/src/test/java/io/sentry/SentryTest.kt b/sentry/src/test/java/io/sentry/SentryTest.kt index e9342c00045..af33994542f 100644 --- a/sentry/src/test/java/io/sentry/SentryTest.kt +++ b/sentry/src/test/java/io/sentry/SentryTest.kt @@ -270,6 +270,40 @@ class SentryTest { assertTrue(File(sentryOptions?.profilingTracesDirPath!!).list()!!.isEmpty()) } + @Test + fun `only old profiles in profilingTracesDirPath should be cleared when profiling is enabled`() { + val tempPath = getTempPath() + val options = SentryOptions().also { + it.dsn = dsn + it.cacheDirPath = tempPath + } + val dir = File(options.profilingTracesDirPath!!) + val oldProfile = File(dir, "oldProfile") + val newProfile = File(dir, "newProfile") + + // Create all files + dir.mkdirs() + oldProfile.createNewFile() + newProfile.createNewFile() + // Make the new profile look like it's created later + newProfile.setLastModified(System.currentTimeMillis() + 10000) + + // Assert both file exist + assertTrue(oldProfile.exists()) + assertTrue(newProfile.exists()) + + Sentry.init { + it.dsn = dsn + it.profilesSampleRate = 1.0 + it.cacheDirPath = tempPath + it.executorService = ImmediateExecutorService() + } + + // Assert only the new profile exists + assertFalse(oldProfile.exists()) + assertTrue(newProfile.exists()) + } + @Test fun `profilingTracesDirPath should not be created and cleared when profiling is disabled`() { val tempPath = getTempPath()