diff --git a/CHANGELOG.md b/CHANGELOG.md index a7b1c62f37f..ec13dc20e51 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - Session Replay: Use main thread looper to schedule replay capture ([#4542](https://github.com/getsentry/sentry-java/pull/4542)) - Use single `LifecycleObserver` and multi-cast it to the integrations interested in lifecycle states ([#4567](https://github.com/getsentry/sentry-java/pull/4567)) +- Prewarm `SentryExecutorService` for better performance at runtime ([#4606](https://github.com/getsentry/sentry-java/pull/4606)) ### Fixes diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/ContextUtils.java b/sentry-android-core/src/main/java/io/sentry/android/core/ContextUtils.java index 8ba0e16b7c6..60ae00f2ead 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/ContextUtils.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/ContextUtils.java @@ -15,6 +15,7 @@ import android.content.pm.PackageInfo; import android.content.pm.PackageManager; import android.os.Build; +import android.os.Handler; import android.util.DisplayMetrics; import io.sentry.ILogger; import io.sentry.SentryLevel; @@ -455,8 +456,10 @@ public static boolean appIsLibraryForComposePreview(final @NotNull Context conte final @NotNull Context context, final @NotNull SentryOptions options, final @Nullable BroadcastReceiver receiver, - final @NotNull IntentFilter filter) { - return registerReceiver(context, new BuildInfoProvider(options.getLogger()), receiver, filter); + final @NotNull IntentFilter filter, + final @Nullable Handler handler) { + return registerReceiver( + context, new BuildInfoProvider(options.getLogger()), receiver, filter, handler); } /** Register an exported BroadcastReceiver, independently from platform version. */ @@ -465,15 +468,17 @@ public static boolean appIsLibraryForComposePreview(final @NotNull Context conte final @NotNull Context context, final @NotNull BuildInfoProvider buildInfoProvider, final @Nullable BroadcastReceiver receiver, - final @NotNull IntentFilter filter) { + final @NotNull IntentFilter filter, + final @Nullable Handler handler) { if (buildInfoProvider.getSdkInfoVersion() >= Build.VERSION_CODES.TIRAMISU) { // From https://developer.android.com/guide/components/broadcasts#context-registered-receivers // If this receiver is listening for broadcasts sent from the system or from other apps, even // other apps that you own—use the RECEIVER_EXPORTED flag. If instead this receiver is // listening only for broadcasts sent by your app, use the RECEIVER_NOT_EXPORTED flag. - return context.registerReceiver(receiver, filter, Context.RECEIVER_NOT_EXPORTED); + return context.registerReceiver( + receiver, filter, null, handler, Context.RECEIVER_NOT_EXPORTED); } else { - return context.registerReceiver(receiver, filter); + return context.registerReceiver(receiver, filter, null, handler); } } diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/DeviceInfoUtil.java b/sentry-android-core/src/main/java/io/sentry/android/core/DeviceInfoUtil.java index 5baf6e2a8d2..7ba321426a1 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/DeviceInfoUtil.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/DeviceInfoUtil.java @@ -275,7 +275,7 @@ private Date getBootTime() { @Nullable private Intent getBatteryIntent() { return ContextUtils.registerReceiver( - context, buildInfoProvider, null, new IntentFilter(Intent.ACTION_BATTERY_CHANGED)); + context, buildInfoProvider, null, new IntentFilter(Intent.ACTION_BATTERY_CHANGED), null); } /** diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java b/sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java index a5e4d398e74..3d4cedb1b53 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/LifecycleWatcher.java @@ -8,6 +8,7 @@ import io.sentry.transport.CurrentDateProvider; import io.sentry.transport.ICurrentDateProvider; import io.sentry.util.AutoClosableReentrantLock; +import io.sentry.util.LazyEvaluator; import java.util.Timer; import java.util.TimerTask; import java.util.concurrent.atomic.AtomicLong; @@ -22,7 +23,7 @@ final class LifecycleWatcher implements AppState.AppStateListener { private final long sessionIntervalMillis; private @Nullable TimerTask timerTask; - private final @NotNull Timer timer = new Timer(true); + private final @NotNull LazyEvaluator timer = new LazyEvaluator<>(() -> new Timer(true)); private final @NotNull AutoClosableReentrantLock timerLock = new AutoClosableReentrantLock(); private final @NotNull IScopes scopes; private final boolean enableSessionTracking; @@ -105,21 +106,19 @@ public void onBackground() { private void scheduleEndSession() { try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) { cancelTask(); - if (timer != null) { - timerTask = - new TimerTask() { - @Override - public void run() { - if (enableSessionTracking) { - scopes.endSession(); - } - scopes.getOptions().getReplayController().stop(); - scopes.getOptions().getContinuousProfiler().close(false); + timerTask = + new TimerTask() { + @Override + public void run() { + if (enableSessionTracking) { + scopes.endSession(); } - }; + scopes.getOptions().getReplayController().stop(); + scopes.getOptions().getContinuousProfiler().close(false); + } + }; - timer.schedule(timerTask, sessionIntervalMillis); - } + timer.getValue().schedule(timerTask, sessionIntervalMillis); } } @@ -152,6 +151,6 @@ TimerTask getTimerTask() { @TestOnly @NotNull Timer getTimer() { - return timer; + return timer.getValue(); } } diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/SystemEventsBreadcrumbsIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/SystemEventsBreadcrumbsIntegration.java index b08b429fdf2..91099d01253 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/SystemEventsBreadcrumbsIntegration.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/SystemEventsBreadcrumbsIntegration.java @@ -25,6 +25,9 @@ import android.content.Intent; import android.content.IntentFilter; import android.os.Bundle; +import android.os.Handler; +import android.os.HandlerThread; +import android.os.Process; import io.sentry.Breadcrumb; import io.sentry.Hint; import io.sentry.IScopes; @@ -63,6 +66,7 @@ public final class SystemEventsBreadcrumbsIntegration private volatile boolean isClosed = false; private volatile boolean isStopped = false; private volatile IntentFilter filter = null; + private volatile HandlerThread handlerThread = null; private final @NotNull AtomicBoolean isReceiverRegistered = new AtomicBoolean(false); private final @NotNull AutoClosableReentrantLock receiverLock = new AutoClosableReentrantLock(); // Track previous battery state to avoid duplicate breadcrumbs when values haven't changed @@ -138,10 +142,19 @@ private void registerReceiver( filter.addAction(item); } } + if (handlerThread == null) { + handlerThread = + new HandlerThread( + "SystemEventsReceiver", Process.THREAD_PRIORITY_BACKGROUND); + handlerThread.start(); + } try { // registerReceiver can throw SecurityException but it's not documented in the // official docs - ContextUtils.registerReceiver(context, options, receiver, filter); + + // onReceive will be called on this handler thread + final @NotNull Handler handler = new Handler(handlerThread.getLooper()); + ContextUtils.registerReceiver(context, options, receiver, filter, handler); if (!isReceiverRegistered.getAndSet(true)) { options .getLogger() @@ -195,6 +208,10 @@ public void close() throws IOException { try (final @NotNull ISentryLifecycleToken ignored = receiverLock.acquire()) { isClosed = true; filter = null; + if (handlerThread != null) { + handlerThread.quit(); + } + handlerThread = null; } AppState.getInstance().removeAppStateListener(this); @@ -293,25 +310,15 @@ public void onReceive(final Context context, final @NotNull Intent intent) { final BatteryState state = batteryState; final long now = System.currentTimeMillis(); - try { - options - .getExecutorService() - .submit( - () -> { - final Breadcrumb breadcrumb = createBreadcrumb(now, intent, action, state); - final Hint hint = new Hint(); - hint.set(ANDROID_INTENT, intent); - scopes.addBreadcrumb(breadcrumb, hint); - }); - } catch (Throwable t) { - // ignored - } + final Breadcrumb breadcrumb = createBreadcrumb(now, intent, action, state); + final Hint hint = new Hint(); + hint.set(ANDROID_INTENT, intent); + scopes.addBreadcrumb(breadcrumb, hint); } // in theory this should be ThreadLocal, but we won't have more than 1 thread accessing it, // so we save some memory here and CPU cycles. 64 is because all intent actions we subscribe for // are less than 64 chars. We also don't care about encoding as those are always UTF. - // TODO: _MULTI_THREADED_EXECUTOR_ private final char[] buf = new char[64]; @TestOnly @@ -365,8 +372,8 @@ String getStringAfterDotFast(final @Nullable String str) { } } else { final Bundle extras = intent.getExtras(); - final Map newExtras = new HashMap<>(); if (extras != null && !extras.isEmpty()) { + final Map newExtras = new HashMap<>(extras.size()); for (String item : extras.keySet()) { try { @SuppressWarnings("deprecation") diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidProfilerTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidProfilerTest.kt index 998a24b3d2c..f4b4da814b8 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidProfilerTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidProfilerTest.kt @@ -80,6 +80,8 @@ class AndroidProfilerTest { override fun close(timeoutMillis: Long) {} override fun isClosed() = false + + override fun prewarm() = Unit } val options = diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt index 0490bc30d09..ec6ba18d65e 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt @@ -89,6 +89,8 @@ class AndroidTransactionProfilerTest { override fun close(timeoutMillis: Long) {} override fun isClosed() = false + + override fun prewarm() = Unit } val options = diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ContextUtilsTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ContextUtilsTest.kt index b9bb6170e44..2e540ee13df 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/ContextUtilsTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/ContextUtilsTest.kt @@ -30,6 +30,7 @@ import kotlin.test.assertTrue import org.junit.runner.RunWith import org.mockito.kotlin.any import org.mockito.kotlin.eq +import org.mockito.kotlin.isNull import org.mockito.kotlin.mock import org.mockito.kotlin.spy import org.mockito.kotlin.verify @@ -221,8 +222,8 @@ class ContextUtilsTest { val filter = mock() val context = mock() whenever(buildInfo.sdkInfoVersion).thenReturn(Build.VERSION_CODES.S) - ContextUtils.registerReceiver(context, buildInfo, receiver, filter) - verify(context).registerReceiver(eq(receiver), eq(filter)) + ContextUtils.registerReceiver(context, buildInfo, receiver, filter, null) + verify(context).registerReceiver(eq(receiver), eq(filter), isNull(), isNull()) } @Test @@ -232,8 +233,15 @@ class ContextUtilsTest { val filter = mock() val context = mock() whenever(buildInfo.sdkInfoVersion).thenReturn(Build.VERSION_CODES.TIRAMISU) - ContextUtils.registerReceiver(context, buildInfo, receiver, filter) - verify(context).registerReceiver(eq(receiver), eq(filter), eq(Context.RECEIVER_NOT_EXPORTED)) + ContextUtils.registerReceiver(context, buildInfo, receiver, filter, null) + verify(context) + .registerReceiver( + eq(receiver), + eq(filter), + isNull(), + isNull(), + eq(Context.RECEIVER_NOT_EXPORTED), + ) } @Test diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/SystemEventsBreadcrumbsIntegrationTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/SystemEventsBreadcrumbsIntegrationTest.kt index 1c17ac7c65b..65eb3cb46f3 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/SystemEventsBreadcrumbsIntegrationTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/SystemEventsBreadcrumbsIntegrationTest.kt @@ -89,7 +89,7 @@ class SystemEventsBreadcrumbsIntegrationTest { sut.register(fixture.scopes, fixture.options) - verify(fixture.context).registerReceiver(any(), any(), any()) + verify(fixture.context).registerReceiver(any(), any(), anyOrNull(), anyOrNull(), any()) assertNotNull(sut.receiver) } @@ -299,7 +299,8 @@ class SystemEventsBreadcrumbsIntegrationTest { @Test fun `Do not crash if registerReceiver throws exception`() { val sut = fixture.getSut() - whenever(fixture.context.registerReceiver(any(), any(), any())).thenThrow(SecurityException()) + whenever(fixture.context.registerReceiver(any(), any(), anyOrNull(), anyOrNull(), any())) + .thenThrow(SecurityException()) sut.register(fixture.scopes, fixture.options) @@ -448,12 +449,13 @@ class SystemEventsBreadcrumbsIntegrationTest { val sut = fixture.getSut() sut.register(fixture.scopes, fixture.options) - verify(fixture.context).registerReceiver(any(), any(), any()) + verify(fixture.context).registerReceiver(any(), any(), anyOrNull(), anyOrNull(), any()) sut.onBackground() sut.onForeground() - verify(fixture.context, times(2)).registerReceiver(any(), any(), any()) + verify(fixture.context, times(2)) + .registerReceiver(any(), any(), anyOrNull(), anyOrNull(), any()) assertNotNull(sut.receiver) } @@ -462,7 +464,7 @@ class SystemEventsBreadcrumbsIntegrationTest { val sut = fixture.getSut() sut.register(fixture.scopes, fixture.options) - verify(fixture.context).registerReceiver(any(), any(), any()) + verify(fixture.context).registerReceiver(any(), any(), anyOrNull(), anyOrNull(), any()) val receiver = sut.receiver sut.onForeground() diff --git a/sentry-test-support/api/sentry-test-support.api b/sentry-test-support/api/sentry-test-support.api index 2623a7813d2..ce9d0506ea6 100644 --- a/sentry-test-support/api/sentry-test-support.api +++ b/sentry-test-support/api/sentry-test-support.api @@ -20,6 +20,7 @@ public final class io/sentry/test/DeferredExecutorService : io/sentry/ISentryExe public fun close (J)V public final fun hasScheduledRunnables ()Z public fun isClosed ()Z + public fun prewarm ()V public final fun runAll ()V public fun schedule (Ljava/lang/Runnable;J)Ljava/util/concurrent/Future; public fun submit (Ljava/lang/Runnable;)Ljava/util/concurrent/Future; @@ -30,6 +31,7 @@ public final class io/sentry/test/ImmediateExecutorService : io/sentry/ISentryEx public fun ()V public fun close (J)V public fun isClosed ()Z + public fun prewarm ()V public fun schedule (Ljava/lang/Runnable;J)Ljava/util/concurrent/Future; public fun submit (Ljava/lang/Runnable;)Ljava/util/concurrent/Future; public fun submit (Ljava/util/concurrent/Callable;)Ljava/util/concurrent/Future; diff --git a/sentry-test-support/src/main/kotlin/io/sentry/test/Mocks.kt b/sentry-test-support/src/main/kotlin/io/sentry/test/Mocks.kt index 49189f183e3..09d5d181ec4 100644 --- a/sentry-test-support/src/main/kotlin/io/sentry/test/Mocks.kt +++ b/sentry-test-support/src/main/kotlin/io/sentry/test/Mocks.kt @@ -35,6 +35,8 @@ class ImmediateExecutorService : ISentryExecutorService { override fun close(timeoutMillis: Long) {} override fun isClosed(): Boolean = false + + override fun prewarm() = Unit } class DeferredExecutorService : ISentryExecutorService { @@ -72,6 +74,8 @@ class DeferredExecutorService : ISentryExecutorService { override fun isClosed(): Boolean = false + override fun prewarm() = Unit + fun hasScheduledRunnables(): Boolean = scheduledRunnables.isNotEmpty() } diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index b08be88fc65..767d0441593 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -1030,6 +1030,7 @@ public abstract interface class io/sentry/ISentryClient { public abstract interface class io/sentry/ISentryExecutorService { public abstract fun close (J)V public abstract fun isClosed ()Z + public abstract fun prewarm ()V public abstract fun schedule (Ljava/lang/Runnable;J)Ljava/util/concurrent/Future; public abstract fun submit (Ljava/lang/Runnable;)Ljava/util/concurrent/Future; public abstract fun submit (Ljava/util/concurrent/Callable;)Ljava/util/concurrent/Future; @@ -2957,8 +2958,10 @@ public final class io/sentry/SentryExceptionFactory { public final class io/sentry/SentryExecutorService : io/sentry/ISentryExecutorService { public fun ()V + public fun (Lio/sentry/SentryOptions;)V public fun close (J)V public fun isClosed ()Z + public fun prewarm ()V public fun schedule (Ljava/lang/Runnable;J)Ljava/util/concurrent/Future; public fun submit (Ljava/lang/Runnable;)Ljava/util/concurrent/Future; public fun submit (Ljava/util/concurrent/Callable;)Ljava/util/concurrent/Future; diff --git a/sentry/build.gradle.kts b/sentry/build.gradle.kts index 6a5a2182a89..bad0ea56e50 100644 --- a/sentry/build.gradle.kts +++ b/sentry/build.gradle.kts @@ -53,6 +53,7 @@ tasks { dependsOn(jacocoTestReport) } test { + jvmArgs("--add-opens", "java.base/java.util.concurrent=ALL-UNNAMED") environment["SENTRY_TEST_PROPERTY"] = "\"some-value\"" environment["SENTRY_TEST_MAP_KEY1"] = "\"value1\"" environment["SENTRY_TEST_MAP_KEY2"] = "value2" diff --git a/sentry/src/main/java/io/sentry/ISentryExecutorService.java b/sentry/src/main/java/io/sentry/ISentryExecutorService.java index 9bdef8db2b7..ffad05361f6 100644 --- a/sentry/src/main/java/io/sentry/ISentryExecutorService.java +++ b/sentry/src/main/java/io/sentry/ISentryExecutorService.java @@ -45,4 +45,10 @@ Future schedule(final @NotNull Runnable runnable, final long delayMillis) * @return If the executorService was previously closed */ boolean isClosed(); + + /** + * Pre-warms the executor service by increasing the initial queue capacity. SHOULD be called + * directly after instantiating this executor service. + */ + void prewarm(); } diff --git a/sentry/src/main/java/io/sentry/NoOpScope.java b/sentry/src/main/java/io/sentry/NoOpScope.java index d996fa29d59..dd1a202b548 100644 --- a/sentry/src/main/java/io/sentry/NoOpScope.java +++ b/sentry/src/main/java/io/sentry/NoOpScope.java @@ -5,6 +5,7 @@ import io.sentry.protocol.Request; import io.sentry.protocol.SentryId; import io.sentry.protocol.User; +import io.sentry.util.LazyEvaluator; import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collection; @@ -20,7 +21,8 @@ public final class NoOpScope implements IScope { private static final NoOpScope instance = new NoOpScope(); - private final @NotNull SentryOptions emptyOptions = SentryOptions.empty(); + private final @NotNull LazyEvaluator emptyOptions = + new LazyEvaluator<>(() -> SentryOptions.empty()); private NoOpScope() {} @@ -229,7 +231,7 @@ public void withTransaction(Scope.@NotNull IWithTransaction callback) {} @ApiStatus.Internal @Override public @NotNull SentryOptions getOptions() { - return emptyOptions; + return emptyOptions.getValue(); } @ApiStatus.Internal diff --git a/sentry/src/main/java/io/sentry/NoOpScopes.java b/sentry/src/main/java/io/sentry/NoOpScopes.java index e1cc49a34a7..34adb15769b 100644 --- a/sentry/src/main/java/io/sentry/NoOpScopes.java +++ b/sentry/src/main/java/io/sentry/NoOpScopes.java @@ -7,6 +7,7 @@ import io.sentry.protocol.SentryTransaction; import io.sentry.protocol.User; import io.sentry.transport.RateLimiter; +import io.sentry.util.LazyEvaluator; import java.util.List; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; @@ -16,7 +17,8 @@ public final class NoOpScopes implements IScopes { private static final NoOpScopes instance = new NoOpScopes(); - private final @NotNull SentryOptions emptyOptions = SentryOptions.empty(); + private final @NotNull LazyEvaluator emptyOptions = + new LazyEvaluator<>(() -> SentryOptions.empty()); private NoOpScopes() {} @@ -274,7 +276,7 @@ public void setActiveSpan(final @Nullable ISpan span) {} @Override public @NotNull SentryOptions getOptions() { - return emptyOptions; + return emptyOptions.getValue(); } @Override diff --git a/sentry/src/main/java/io/sentry/NoOpSentryExecutorService.java b/sentry/src/main/java/io/sentry/NoOpSentryExecutorService.java index 735c0dc29a3..d1dc6b207e5 100644 --- a/sentry/src/main/java/io/sentry/NoOpSentryExecutorService.java +++ b/sentry/src/main/java/io/sentry/NoOpSentryExecutorService.java @@ -36,4 +36,7 @@ public void close(long timeoutMillis) {} public boolean isClosed() { return false; } + + @Override + public void prewarm() {} } diff --git a/sentry/src/main/java/io/sentry/Sentry.java b/sentry/src/main/java/io/sentry/Sentry.java index f24ecab9e78..c462f91025b 100644 --- a/sentry/src/main/java/io/sentry/Sentry.java +++ b/sentry/src/main/java/io/sentry/Sentry.java @@ -343,7 +343,8 @@ private static void init(final @NotNull SentryOptions options, final boolean glo // to // set a new one if (options.getExecutorService().isClosed()) { - options.setExecutorService(new SentryExecutorService()); + options.setExecutorService(new SentryExecutorService(options)); + options.getExecutorService().prewarm(); } // when integrations are registered on Scopes ctor and async integrations are fired, // it might and actually happened that integrations called captureSomething diff --git a/sentry/src/main/java/io/sentry/SentryExecutorService.java b/sentry/src/main/java/io/sentry/SentryExecutorService.java index bb08e51b3a0..3fe262b5538 100644 --- a/sentry/src/main/java/io/sentry/SentryExecutorService.java +++ b/sentry/src/main/java/io/sentry/SentryExecutorService.java @@ -2,43 +2,97 @@ import io.sentry.util.AutoClosableReentrantLock; import java.util.concurrent.Callable; -import java.util.concurrent.Executors; +import java.util.concurrent.CancellationException; import java.util.concurrent.Future; -import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.RejectedExecutionException; +import java.util.concurrent.ScheduledThreadPoolExecutor; import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeUnit; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; import org.jetbrains.annotations.TestOnly; @ApiStatus.Internal public final class SentryExecutorService implements ISentryExecutorService { - private final @NotNull ScheduledExecutorService executorService; + /** + * ScheduledThreadPoolExecutor grows work queue by 50% each time. With the initial capacity of 16 + * it will have to resize 4 times to reach 40, which is a decent middle-ground for prewarming. + * This will prevent from growing in unexpected areas of the SDK. + */ + private static final int INITIAL_QUEUE_SIZE = 40; + + /** + * By default, the work queue is unbounded so it can grow as much as the memory allows. We want to + * limit it by 271 which would be x8 times growth from the default initial capacity. + */ + private static final int MAX_QUEUE_SIZE = 271; + + private final @NotNull ScheduledThreadPoolExecutor executorService; private final @NotNull AutoClosableReentrantLock lock = new AutoClosableReentrantLock(); + @SuppressWarnings("UnnecessaryLambda") + private final @NotNull Runnable dummyRunnable = () -> {}; + + private final @Nullable SentryOptions options; + @TestOnly - SentryExecutorService(final @NotNull ScheduledExecutorService executorService) { + SentryExecutorService( + final @NotNull ScheduledThreadPoolExecutor executorService, + final @Nullable SentryOptions options) { this.executorService = executorService; + this.options = options; + } + + public SentryExecutorService(final @Nullable SentryOptions options) { + this(new ScheduledThreadPoolExecutor(1, new SentryExecutorServiceThreadFactory()), options); } public SentryExecutorService() { - this(Executors.newSingleThreadScheduledExecutor(new SentryExecutorServiceThreadFactory())); + this(new ScheduledThreadPoolExecutor(1, new SentryExecutorServiceThreadFactory()), null); } @Override public @NotNull Future submit(final @NotNull Runnable runnable) { - return executorService.submit(runnable); + if (executorService.getQueue().size() < MAX_QUEUE_SIZE) { + return executorService.submit(runnable); + } + // TODO: maybe RejectedExecutionException? + if (options != null) { + options + .getLogger() + .log(SentryLevel.WARNING, "Task " + runnable + " rejected from " + executorService); + } + return new CancelledFuture<>(); } @Override public @NotNull Future submit(final @NotNull Callable callable) { - return executorService.submit(callable); + if (executorService.getQueue().size() < MAX_QUEUE_SIZE) { + return executorService.submit(callable); + } + // TODO: maybe RejectedExecutionException? + if (options != null) { + options + .getLogger() + .log(SentryLevel.WARNING, "Task " + callable + " rejected from " + executorService); + } + return new CancelledFuture<>(); } @Override public @NotNull Future schedule(final @NotNull Runnable runnable, final long delayMillis) { - return executorService.schedule(runnable, delayMillis, TimeUnit.MILLISECONDS); + if (executorService.getQueue().size() < MAX_QUEUE_SIZE) { + return executorService.schedule(runnable, delayMillis, TimeUnit.MILLISECONDS); + } + // TODO: maybe RejectedExecutionException? + if (options != null) { + options + .getLogger() + .log(SentryLevel.WARNING, "Task " + runnable + " rejected from " + executorService); + } + return new CancelledFuture<>(); } @Override @@ -65,6 +119,25 @@ public boolean isClosed() { } } + @SuppressWarnings({"FutureReturnValueIgnored"}) + @Override + public void prewarm() { + executorService.submit( + () -> { + try { + // schedule a bunch of dummy runnables in the future that will never execute to trigger + // queue growth and then purge the queue + for (int i = 0; i < INITIAL_QUEUE_SIZE; i++) { + final Future future = executorService.schedule(dummyRunnable, 365L, TimeUnit.DAYS); + future.cancel(true); + } + executorService.purge(); + } catch (RejectedExecutionException ignored) { + // ignore + } + }); + } + private static final class SentryExecutorServiceThreadFactory implements ThreadFactory { private int cnt; @@ -75,4 +148,31 @@ private static final class SentryExecutorServiceThreadFactory implements ThreadF return ret; } } + + private static final class CancelledFuture implements Future { + @Override + public boolean cancel(final boolean mayInterruptIfRunning) { + return true; + } + + @Override + public boolean isCancelled() { + return true; + } + + @Override + public boolean isDone() { + return true; + } + + @Override + public T get() { + throw new CancellationException(); + } + + @Override + public T get(final long timeout, final @NotNull TimeUnit unit) { + throw new CancellationException(); + } + } } diff --git a/sentry/src/main/java/io/sentry/SentryOptions.java b/sentry/src/main/java/io/sentry/SentryOptions.java index 1ca089d7885..0a8227408ad 100644 --- a/sentry/src/main/java/io/sentry/SentryOptions.java +++ b/sentry/src/main/java/io/sentry/SentryOptions.java @@ -3072,7 +3072,8 @@ private SentryOptions(final boolean empty) { setSpanFactory(SpanFactoryFactory.create(new LoadClass(), NoOpLogger.getInstance())); // SentryExecutorService should be initialized before any // SendCachedEventFireAndForgetIntegration - executorService = new SentryExecutorService(); + executorService = new SentryExecutorService(this); + executorService.prewarm(); // UncaughtExceptionHandlerIntegration should be inited before any other Integration. // if there's an error on the setup, we are able to capture it diff --git a/sentry/src/main/java/io/sentry/SpotlightIntegration.java b/sentry/src/main/java/io/sentry/SpotlightIntegration.java index 910259ad131..1eb9ed83ae0 100644 --- a/sentry/src/main/java/io/sentry/SpotlightIntegration.java +++ b/sentry/src/main/java/io/sentry/SpotlightIntegration.java @@ -32,7 +32,7 @@ public void register(@NotNull IScopes scopes, @NotNull SentryOptions options) { this.logger = options.getLogger(); if (options.getBeforeEnvelopeCallback() == null && options.isEnableSpotlight()) { - executorService = new SentryExecutorService(); + executorService = new SentryExecutorService(options); options.setBeforeEnvelopeCallback(this); logger.log(DEBUG, "SpotlightIntegration enabled."); addIntegrationToSdkVersion("Spotlight"); diff --git a/sentry/src/main/java/io/sentry/logger/LoggerBatchProcessor.java b/sentry/src/main/java/io/sentry/logger/LoggerBatchProcessor.java index 5aa03a74118..3d760263f38 100644 --- a/sentry/src/main/java/io/sentry/logger/LoggerBatchProcessor.java +++ b/sentry/src/main/java/io/sentry/logger/LoggerBatchProcessor.java @@ -40,7 +40,7 @@ public LoggerBatchProcessor( this.options = options; this.client = client; this.queue = new ConcurrentLinkedQueue<>(); - this.executorService = new SentryExecutorService(); + this.executorService = new SentryExecutorService(options); } @Override diff --git a/sentry/src/test/java/io/sentry/SentryExecutorServiceTest.kt b/sentry/src/test/java/io/sentry/SentryExecutorServiceTest.kt index 18cc73986a4..19dd60469cc 100644 --- a/sentry/src/test/java/io/sentry/SentryExecutorServiceTest.kt +++ b/sentry/src/test/java/io/sentry/SentryExecutorServiceTest.kt @@ -1,13 +1,20 @@ package io.sentry -import java.util.concurrent.Executors -import java.util.concurrent.ScheduledExecutorService +import io.sentry.test.getProperty +import java.util.concurrent.BlockingQueue +import java.util.concurrent.Callable +import java.util.concurrent.CancellationException +import java.util.concurrent.LinkedBlockingQueue +import java.util.concurrent.ScheduledThreadPoolExecutor import java.util.concurrent.atomic.AtomicBoolean import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertFalse import kotlin.test.assertTrue import org.awaitility.kotlin.await import org.mockito.kotlin.any +import org.mockito.kotlin.doReturn import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.verify @@ -16,24 +23,24 @@ import org.mockito.kotlin.whenever class SentryExecutorServiceTest { @Test fun `SentryExecutorService forwards submit call to ExecutorService`() { - val executor = mock() - val sentryExecutor = SentryExecutorService(executor) + val executor = mock { on { queue } doReturn LinkedBlockingQueue() } + val sentryExecutor = SentryExecutorService(executor, null) sentryExecutor.submit {} verify(executor).submit(any()) } @Test fun `SentryExecutorService forwards schedule call to ExecutorService`() { - val executor = mock() - val sentryExecutor = SentryExecutorService(executor) + val executor = mock { on { queue } doReturn LinkedBlockingQueue() } + val sentryExecutor = SentryExecutorService(executor, null) sentryExecutor.schedule({}, 0L) verify(executor).schedule(any(), any(), any()) } @Test fun `SentryExecutorService forwards close call to ExecutorService`() { - val executor = mock() - val sentryExecutor = SentryExecutorService(executor) + val executor = mock() + val sentryExecutor = SentryExecutorService(executor, null) whenever(executor.isShutdown).thenReturn(false) whenever(executor.awaitTermination(any(), any())).thenReturn(true) sentryExecutor.close(15000) @@ -42,8 +49,8 @@ class SentryExecutorServiceTest { @Test fun `SentryExecutorService forwards close and call shutdownNow if not enough time`() { - val executor = mock() - val sentryExecutor = SentryExecutorService(executor) + val executor = mock() + val sentryExecutor = SentryExecutorService(executor, null) whenever(executor.isShutdown).thenReturn(false) whenever(executor.awaitTermination(any(), any())).thenReturn(false) sentryExecutor.close(15000) @@ -52,8 +59,8 @@ class SentryExecutorServiceTest { @Test fun `SentryExecutorService forwards close and call shutdownNow if await throws`() { - val executor = mock() - val sentryExecutor = SentryExecutorService(executor) + val executor = mock() + val sentryExecutor = SentryExecutorService(executor, null) whenever(executor.isShutdown).thenReturn(false) whenever(executor.awaitTermination(any(), any())).thenThrow(InterruptedException()) sentryExecutor.close(15000) @@ -62,8 +69,8 @@ class SentryExecutorServiceTest { @Test fun `SentryExecutorService forwards close but do not shutdown if its already closed`() { - val executor = mock() - val sentryExecutor = SentryExecutorService(executor) + val executor = mock() + val sentryExecutor = SentryExecutorService(executor, null) whenever(executor.isShutdown).thenReturn(true) sentryExecutor.close(15000) verify(executor, never()).shutdown() @@ -71,8 +78,8 @@ class SentryExecutorServiceTest { @Test fun `SentryExecutorService forwards close call to ExecutorService and close it`() { - val executor = Executors.newSingleThreadScheduledExecutor() - val sentryExecutor = SentryExecutorService(executor) + val executor = ScheduledThreadPoolExecutor(1) + val sentryExecutor = SentryExecutorService(executor, null) sentryExecutor.close(15000) assertTrue(executor.isShutdown) } @@ -88,17 +95,134 @@ class SentryExecutorServiceTest { @Test fun `SentryExecutorService isClosed returns true if executor is shutdown`() { - val executor = mock() - val sentryExecutor = SentryExecutorService(executor) + val executor = mock() + val sentryExecutor = SentryExecutorService(executor, null) whenever(executor.isShutdown).thenReturn(true) assertTrue(sentryExecutor.isClosed) } @Test fun `SentryExecutorService isClosed returns false if executor is not shutdown`() { - val executor = mock() - val sentryExecutor = SentryExecutorService(executor) + val executor = mock() + val sentryExecutor = SentryExecutorService(executor, null) whenever(executor.isShutdown).thenReturn(false) assertFalse(sentryExecutor.isClosed) } + + @Test + fun `SentryExecutorService submit runnable returns cancelled future when queue size exceeds limit`() { + val queue = mock>() + whenever(queue.size).thenReturn(272) // Above MAX_QUEUE_SIZE (271) + + val executor = mock { on { getQueue() } doReturn queue } + + val options = mock() + val logger = mock() + whenever(options.logger).thenReturn(logger) + + val sentryExecutor = SentryExecutorService(executor, options) + val future = sentryExecutor.submit {} + + assertTrue(future.isCancelled) + assertTrue(future.isDone) + assertFailsWith { future.get() } + verify(executor, never()).submit(any()) + verify(logger).log(any(), any()) + } + + @Test + fun `SentryExecutorService submit runnable accepts when queue size is within limit`() { + val queue = mock>() + whenever(queue.size).thenReturn(270) // Below MAX_QUEUE_SIZE (271) + + val executor = mock { on { getQueue() } doReturn queue } + + val sentryExecutor = SentryExecutorService(executor, null) + sentryExecutor.submit {} + + verify(executor).submit(any()) + } + + @Test + fun `SentryExecutorService submit callable returns cancelled future when queue size exceeds limit`() { + val queue = mock>() + whenever(queue.size).thenReturn(272) // Above MAX_QUEUE_SIZE (271) + + val executor = mock { on { getQueue() } doReturn queue } + + val options = mock() + val logger = mock() + whenever(options.logger).thenReturn(logger) + + val sentryExecutor = SentryExecutorService(executor, options) + val future = sentryExecutor.submit(Callable { "result" }) + + assertTrue(future.isCancelled) + assertTrue(future.isDone) + assertFailsWith { future.get() } + verify(executor, never()).submit(any>()) + verify(logger).log(any(), any()) + } + + @Test + fun `SentryExecutorService submit callable accepts when queue size is within limit`() { + val queue = mock>() + whenever(queue.size).thenReturn(270) // Below MAX_QUEUE_SIZE (271) + + val executor = mock { on { getQueue() } doReturn queue } + + val sentryExecutor = SentryExecutorService(executor, null) + sentryExecutor.submit(Callable { "result" }) + + verify(executor).submit(any>()) + } + + @Test + fun `SentryExecutorService schedule returns cancelled future when queue size exceeds limit`() { + val queue = mock>() + whenever(queue.size).thenReturn(272) // Above MAX_QUEUE_SIZE (271) + + val executor = mock { on { getQueue() } doReturn queue } + + val options = mock() + val logger = mock() + whenever(options.logger).thenReturn(logger) + + val sentryExecutor = SentryExecutorService(executor, options) + val future = sentryExecutor.schedule({}, 1000L) + + assertTrue(future.isCancelled) + assertTrue(future.isDone) + assertFailsWith { future.get() } + verify(executor, never()).schedule(any(), any(), any()) + verify(logger).log(any(), any()) + } + + @Test + fun `SentryExecutorService schedule accepts when queue size is within limit`() { + val queue = mock>() + whenever(queue.size).thenReturn(270) // Below MAX_QUEUE_SIZE (271) + + val executor = mock { on { getQueue() } doReturn queue } + + val sentryExecutor = SentryExecutorService(executor, null) + sentryExecutor.schedule({}, 1000L) + + verify(executor).schedule(any(), any(), any()) + } + + @Test + fun `SentryExecutorService prewarm schedules dummy tasks and clears queue`() { + val executor = ScheduledThreadPoolExecutor(1) + + val sentryExecutor = SentryExecutorService(executor, null) + sentryExecutor.prewarm() + + Thread.sleep(1000) + + // the internal queue/array should be resized 4 times to 54 + assertEquals(54, (executor.queue.getProperty("queue") as Array<*>).size) + // the queue should be empty + assertEquals(0, executor.queue.size) + } }