diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ff6b1f542e..5c5b1b035f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,8 @@ - Session Replay: Fix `SentryOkHttpInterceptor` hanging forever on responses whose body has no known length, such as Server-Sent Events or a gzipped response, when network details are enabled for the URL ([#6231](https://github.com/getsentry/sentry-java/pull/6231)) - Session Replay: `SentryOkHttpInterceptor` now captures network request and response details only while Session Replay is recording or buffering ([#6486](https://github.com/getsentry/sentry-java/pull/6486)) - Stop marking an OkHttp call span as failed when a connection attempt to one of the host's addresses fails but OkHttp connects on another ([#6472](https://github.com/getsentry/sentry-java/pull/6472)) +- Stop blocking the main thread while the frame metrics thread starts when the first activity transaction begins ([#6500](https://github.com/getsentry/sentry-java/pull/6500)) + - On a busy device during cold start this could stall the launcher activity's `onCreate` for hundreds of milliseconds. ### Dependencies diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/SpanFrameMetricsCollector.java b/sentry-android-core/src/main/java/io/sentry/android/core/SpanFrameMetricsCollector.java index 074a4a6ea51..9eb854ca0bf 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/SpanFrameMetricsCollector.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/SpanFrameMetricsCollector.java @@ -74,6 +74,10 @@ public SpanFrameMetricsCollector( this.frameMetricsCollector = frameMetricsCollector; enabled = options.isEnablePerformanceV2() && options.isEnableFramesTracking(); + if (enabled && options.isTracingEnabled()) { + // Start the frame metrics thread now, so it's ready before the first activity's first frame. + frameMetricsCollector.prepare(); + } } @Override diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollector.java b/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollector.java index 2c0ae246558..bc3bff9b700 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollector.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollector.java @@ -35,6 +35,7 @@ import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.jetbrains.annotations.VisibleForTesting; @ApiStatus.Internal public final class SentryFrameMetricsCollector implements Application.ActivityLifecycleCallbacks { @@ -48,8 +49,10 @@ public final class SentryFrameMetricsCollector implements Application.ActivityLi private final @NotNull ILogger logger; private volatile @Nullable Handler handler; + private volatile boolean handlerThreadStarted = false; private final @NotNull AutoClosableReentrantLock handlerLock = new AutoClosableReentrantLock(); - private @Nullable WeakReference currentWindow; + // volatile: read from the HandlerThread in onHandlerReady while set on the main thread + private volatile @Nullable WeakReference currentWindow; private final @NotNull Map listenerMap = new ConcurrentHashMap<>(); private boolean isAvailable = false; @@ -116,8 +119,8 @@ public SentryFrameMetricsCollector( } isAvailable = true; - // The frame metrics HandlerThread is started lazily on the first startCollection() call. - // Starting it here would block the main thread on HandlerThread.getLooper() during SDK init. + // The frame metrics HandlerThread is started by prepare() or the first startCollection() call, + // and never waited on: see ensureHandlerThreadStarted(). // We have to register the lifecycle callback, even if no profile is started, otherwise when we // start a profile, we wouldn't have the current activity and couldn't get the frameMetrics. @@ -294,26 +297,51 @@ public void onActivityDestroyed(@NotNull Activity activity) {} } /** - * Lazily starts the background HandlerThread used to receive frame metrics. Deferred out of the - * constructor because {@link HandlerThread#getLooper()} blocks the caller (the main thread during - * SDK init) until the thread is ready, and the handler is only needed once collection starts. + * Starts the background HandlerThread ahead of the first {@link #startCollection} call, so its + * looper is ready by the time the first frame is drawn. Does not block. + */ + public void prepare() { + if (isAvailable) { + ensureHandlerThreadStarted(); + } + } + + /** + * Starts the background HandlerThread used to receive frame metrics without waiting for it: the + * handler is created on the new thread in {@link HandlerThread#onLooperPrepared()}. Calling + * {@link HandlerThread#getLooper()} here would block the caller (the main thread) until the new + * thread is scheduled, which takes hundreds of ms on a busy device. */ private void ensureHandlerThreadStarted() { - if (handler != null) { + if (handlerThreadStarted) { return; } try (final @NotNull ISentryLifecycleToken ignored = handlerLock.acquire()) { - if (handler == null) { - final HandlerThread handlerThread = - new HandlerThread("io.sentry.android.core.internal.util.SentryFrameMetricsCollector"); - handlerThread.setUncaughtExceptionHandler( - (thread, e) -> logger.log(SentryLevel.ERROR, "Error during frames measurements.", e)); - handlerThread.start(); - handler = new Handler(handlerThread.getLooper()); + if (handlerThreadStarted) { + return; } + handlerThreadStarted = true; + final HandlerThread handlerThread = + new HandlerThread("io.sentry.android.core.internal.util.SentryFrameMetricsCollector") { + @Override + protected void onLooperPrepared() { + onHandlerReady(new Handler(getLooper())); + } + }; + handlerThread.setUncaughtExceptionHandler( + (thread, e) -> logger.log(SentryLevel.ERROR, "Error during frames measurements.", e)); + handlerThread.start(); } } + @VisibleForTesting + void onHandlerReady(final @NotNull Handler handler) { + this.handler = handler; + // startCollection() or an activity start may have happened before the handler existed, in + // which case trackCurrentWindow() skipped attaching the listener + trackCurrentWindow(); + } + public void stopCollection(final @Nullable String listenerId) { if (!isAvailable) { return; @@ -321,7 +349,9 @@ public void stopCollection(final @Nullable String listenerId) { if (listenerId != null) { listenerMap.remove(listenerId); } - Window window = currentWindow != null ? currentWindow.get() : null; + // read the volatile field once: onActivityStopped may null it concurrently + final @Nullable WeakReference windowRef = currentWindow; + final @Nullable Window window = windowRef != null ? windowRef.get() : null; if (window != null && listenerMap.isEmpty()) { stopTrackingWindow(window); } @@ -355,7 +385,9 @@ private void setCurrentWindow(final @NotNull Window window) { @SuppressLint("NewApi") private void trackCurrentWindow() { - @Nullable Window window = currentWindow != null ? currentWindow.get() : null; + // read the volatile field once: onActivityStopped may null it concurrently + final @Nullable WeakReference windowRef = currentWindow; + final @Nullable Window window = windowRef != null ? windowRef.get() : null; if (window == null || !isAvailable) { return; } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/SpanFrameMetricsCollectorTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/SpanFrameMetricsCollectorTest.kt index 2b6f19a8d31..d5932b75b90 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/SpanFrameMetricsCollectorTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/SpanFrameMetricsCollectorTest.kt @@ -27,7 +27,10 @@ class SpanFrameMetricsCollectorTest { var timeNanos = 0L var lastKnownChoreographerFrameTimeNanos = 0L - fun getSut(enabled: Boolean = true): SpanFrameMetricsCollector { + fun getSut( + enabled: Boolean = true, + tracesSampleRate: Double? = 1.0, + ): SpanFrameMetricsCollector { whenever(frameMetricsCollector.startCollection(any())) .thenReturn(UUID.randomUUID().toString()) whenever(frameMetricsCollector.getLastKnownFrameStartTimeNanos()).thenAnswer { @@ -37,6 +40,7 @@ class SpanFrameMetricsCollectorTest { options.isEnableFramesTracking = enabled options.isEnablePerformanceV2 = enabled options.dateProvider = SentryAndroidDateProvider() + options.tracesSampleRate = tracesSampleRate return SpanFrameMetricsCollector(options, frameMetricsCollector) } @@ -90,6 +94,24 @@ class SpanFrameMetricsCollectorTest { verify(fixture.frameMetricsCollector, never()).startCollection(any()) } + @Test + fun `When enabled and tracing is enabled, it prepares the frame metrics collector`() { + fixture.getSut() + verify(fixture.frameMetricsCollector).prepare() + } + + @Test + fun `When tracing is disabled, it doesn't prepare the frame metrics collector`() { + fixture.getSut(tracesSampleRate = null) + verify(fixture.frameMetricsCollector, never()).prepare() + } + + @Test + fun `When disabled, it doesn't prepare the frame metrics collector`() { + fixture.getSut(enabled = false) + verify(fixture.frameMetricsCollector, never()).prepare() + } + @Test fun `If disabled but a span is launched, it doesn't register a listener`() { val sut = fixture.getSut(enabled = false) diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollectorTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollectorTest.kt index 334ce229066..fb4dbdc67fd 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollectorTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/SentryFrameMetricsCollectorTest.kt @@ -83,17 +83,30 @@ class SentryFrameMetricsCollectorTest { fun getSut( context: Context, buildInfoProvider: BuildInfoProvider = buildInfo, + prepare: Boolean = true, ): SentryFrameMetricsCollector { whenever(activity.window).thenReturn(window) whenever(activity2.window).thenReturn(window2) addOnFrameMetricsAvailableListenerCounter = 0 removeOnFrameMetricsAvailableListenerCounter = 0 - return SentryFrameMetricsCollector( - context, - options, - buildInfoProvider, - windowFrameMetricsManager, - ) + val collector = + SentryFrameMetricsCollector(context, options, buildInfoProvider, windowFrameMetricsManager) + if (prepare) { + collector.prepare() + if (collector.getProperty("handlerThreadStarted")) { + awaitHandler(collector) + } + } + return collector + } + + // the handler is created asynchronously on the HandlerThread once its looper is prepared + fun awaitHandler(collector: SentryFrameMetricsCollector) { + val deadline = System.currentTimeMillis() + 5_000 + while (collector.getProperty("handler") == null) { + check(System.currentTimeMillis() < deadline) { "frame metrics handler was never created" } + Thread.sleep(1) + } } } @@ -142,12 +155,37 @@ class SentryFrameMetricsCollectorTest { @Test fun `handler thread is started lazily on first startCollection`() { - val collector = fixture.getSut(context) - // not started during construction (would block the main thread on getLooper at SDK init) - assertNull(collector.getProperty("handler")) + val collector = fixture.getSut(context, prepare = false) + assertFalse(collector.getProperty("handlerThreadStarted")) collector.startCollection(mock()) - assertNotNull(collector.getProperty("handler")) + assertTrue(collector.getProperty("handlerThreadStarted")) + fixture.awaitHandler(collector) + } + + @Test + fun `prepare starts the handler thread before startCollection`() { + val collector = fixture.getSut(context, prepare = false) + + collector.prepare() + assertTrue(collector.getProperty("handlerThreadStarted")) + fixture.awaitHandler(collector) + } + + @Test + fun `listener is attached once the handler is ready if collection started before it`() { + val collector = fixture.getSut(context, prepare = false) + // pretend the HandlerThread is started but its looper isn't prepared yet + collector.injectForField("handlerThreadStarted", true) + + collector.startCollection(mock()) + collector.onActivityStarted(fixture.activity) + Shadows.shadowOf(Looper.getMainLooper()).idle() + assertEquals(0, fixture.addOnFrameMetricsAvailableListenerCounter) + + collector.onHandlerReady(Handler(Looper.getMainLooper())) + Shadows.shadowOf(Looper.getMainLooper()).idle() + assertEquals(1, fixture.addOnFrameMetricsAvailableListenerCounter) } @Test