From 15bb186bda8444f0900410d7c55aab1025ce3e70 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Wed, 16 Nov 2022 15:57:47 +0100 Subject: [PATCH] Fix ANR on dropped uncaught exception events (#2368) --- CHANGELOG.md | 1 + .../UncaughtExceptionHandlerIntegration.java | 22 +- ...UncaughtExceptionHandlerIntegrationTest.kt | 225 +++++++++++------- 3 files changed, 149 insertions(+), 99 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index af924736a8..3e75803e90 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ - Fix `Gpu.vendorId` should be a String ([#2343](https://github.com/getsentry/sentry-java/pull/2343)) - Don't set device name on Android if `sendDefaultPii` is disabled ([#2354](https://github.com/getsentry/sentry-java/pull/2354)) - Fix corrupted UUID on Motorola devices ([#2363](https://github.com/getsentry/sentry-java/pull/2363)) +- Fix ANR on dropped uncaught exception events ([#2368](https://github.com/getsentry/sentry-java/pull/2368)) ### Features diff --git a/sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java b/sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java index 9058d536b9..c13e6ce580 100644 --- a/sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java +++ b/sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java @@ -7,6 +7,7 @@ import io.sentry.hints.Flushable; import io.sentry.hints.SessionEnd; import io.sentry.protocol.Mechanism; +import io.sentry.protocol.SentryId; import io.sentry.util.HintUtils; import io.sentry.util.Objects; import java.io.Closeable; @@ -97,15 +98,18 @@ public void uncaughtException(Thread thread, Throwable thrown) { final Hint hint = HintUtils.createWithTypeCheckHint(exceptionHint); - hub.captureEvent(event, hint); - // Block until the event is flushed to disk - if (!exceptionHint.waitFlush()) { - options - .getLogger() - .log( - SentryLevel.WARNING, - "Timed out waiting to flush event to disk before crashing. Event: %s", - event.getEventId()); + final @NotNull SentryId sentryId = hub.captureEvent(event, hint); + final boolean isEventDropped = sentryId.equals(SentryId.EMPTY_ID); + if (!isEventDropped) { + // Block until the event is flushed to disk + if (!exceptionHint.waitFlush()) { + options + .getLogger() + .log( + SentryLevel.WARNING, + "Timed out waiting to flush event to disk before crashing. Event: %s", + event.getEventId()); + } } } catch (Throwable e) { options diff --git a/sentry/src/test/java/io/sentry/UncaughtExceptionHandlerIntegrationTest.kt b/sentry/src/test/java/io/sentry/UncaughtExceptionHandlerIntegrationTest.kt index 4a425e01b7..db37ea631c 100644 --- a/sentry/src/test/java/io/sentry/UncaughtExceptionHandlerIntegrationTest.kt +++ b/sentry/src/test/java/io/sentry/UncaughtExceptionHandlerIntegrationTest.kt @@ -1,91 +1,97 @@ package io.sentry import io.sentry.exception.ExceptionMechanismException +import io.sentry.hints.DiskFlushNotification import io.sentry.protocol.SentryId -import io.sentry.util.noFlushTimeout +import io.sentry.util.HintUtils import org.mockito.kotlin.any +import org.mockito.kotlin.argThat import org.mockito.kotlin.argWhere import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import java.io.ByteArrayOutputStream -import java.io.File import java.io.PrintStream import java.nio.file.Files -import kotlin.test.AfterTest -import kotlin.test.BeforeTest +import kotlin.concurrent.thread import kotlin.test.Test import kotlin.test.assertNotNull import kotlin.test.assertTrue class UncaughtExceptionHandlerIntegrationTest { - private lateinit var file: File - - @BeforeTest - fun `set up`() { - file = Files.createTempDirectory("sentry-disk-cache-test").toAbsolutePath().toFile() + class Fixture { + val file = Files.createTempDirectory("sentry-disk-cache-test").toAbsolutePath().toFile() + internal val handler = mock() + val defaultHandler = mock() + val thread = mock() + val throwable = Throwable("test") + val hub = mock() + val options = SentryOptions() + val logger = mock() + + fun getSut( + flushTimeoutMillis: Long = 0L, + hasDefaultHandler: Boolean = false, + enableUncaughtExceptionHandler: Boolean = true, + isPrintUncaughtStackTrace: Boolean = false + ): UncaughtExceptionHandlerIntegration { + options.flushTimeoutMillis = flushTimeoutMillis + options.isEnableUncaughtExceptionHandler = enableUncaughtExceptionHandler + options.isPrintUncaughtStackTrace = isPrintUncaughtStackTrace + options.setLogger(logger) + options.isDebug = true + whenever(handler.defaultUncaughtExceptionHandler).thenReturn(if (hasDefaultHandler) defaultHandler else null) + return UncaughtExceptionHandlerIntegration(handler) + } } - @AfterTest - fun shutdown() { - Files.delete(file.toPath()) - } + private val fixture = Fixture() @Test fun `when UncaughtExceptionHandlerIntegration is initialized, uncaught handler is unchanged`() { - val handlerMock = mock() - UncaughtExceptionHandlerIntegration(handlerMock) - verify(handlerMock, never()).defaultUncaughtExceptionHandler = any() + fixture.getSut(isPrintUncaughtStackTrace = false) + + verify(fixture.handler, never()).defaultUncaughtExceptionHandler = any() } @Test fun `when uncaughtException is called, sentry captures exception`() { - val handlerMock = mock() - val threadMock = mock() - val throwableMock = mock() - val hubMock = mock() - val options = SentryOptions().noFlushTimeout() - val sut = UncaughtExceptionHandlerIntegration(handlerMock) - sut.register(hubMock, options) - sut.uncaughtException(threadMock, throwableMock) - verify(hubMock).captureEvent(any(), any()) + val sut = fixture.getSut(isPrintUncaughtStackTrace = false) + + sut.register(fixture.hub, fixture.options) + sut.uncaughtException(fixture.thread, fixture.throwable) + + verify(fixture.hub).captureEvent(any(), any()) } @Test fun `when register is called, current handler is not lost`() { - val handlerMock = mock() - val threadMock = mock() - val throwableMock = mock() - val defaultHandlerMock = mock() - whenever(handlerMock.defaultUncaughtExceptionHandler).thenReturn(defaultHandlerMock) - val hubMock = mock() - val options = SentryOptions().noFlushTimeout() - val sut = UncaughtExceptionHandlerIntegration(handlerMock) - sut.register(hubMock, options) - sut.uncaughtException(threadMock, throwableMock) - verify(defaultHandlerMock).uncaughtException(threadMock, throwableMock) + val sut = fixture.getSut(hasDefaultHandler = true, isPrintUncaughtStackTrace = false) + + sut.register(fixture.hub, fixture.options) + sut.uncaughtException(fixture.thread, fixture.throwable) + + verify(fixture.defaultHandler).uncaughtException(fixture.thread, fixture.throwable) } @Test fun `when uncaughtException is called, exception captured has handled=false`() { - val handlerMock = mock() - val threadMock = mock() - val throwableMock = mock() - val hubMock = mock() - whenever(hubMock.captureException(any())).thenAnswer { invocation -> - val e = (invocation.arguments[1] as ExceptionMechanismException) + whenever(fixture.hub.captureException(any())).thenAnswer { invocation -> + val e = invocation.getArgument(1) assertNotNull(e) assertNotNull(e.exceptionMechanism) assertTrue(e.exceptionMechanism.isHandled!!) SentryId.EMPTY_ID } - val options = SentryOptions().noFlushTimeout() - val sut = UncaughtExceptionHandlerIntegration(handlerMock) - sut.register(hubMock, options) - sut.uncaughtException(threadMock, throwableMock) - verify(hubMock).captureEvent(any(), any()) + + val sut = fixture.getSut(isPrintUncaughtStackTrace = false) + + sut.register(fixture.hub, fixture.options) + sut.uncaughtException(fixture.thread, fixture.throwable) + + verify(fixture.hub).captureEvent(any(), any()) } @Test @@ -94,63 +100,57 @@ class UncaughtExceptionHandlerIntegrationTest { val options = SentryOptions() options.dsn = "https://key@sentry.io/proj" options.addIntegration(integrationMock) - options.cacheDirPath = file.absolutePath + options.cacheDirPath = fixture.file.absolutePath options.setSerializer(mock()) -// val expected = HubAdapter.getInstance() val hub = Hub(options) -// verify(integrationMock).register(expected, options) hub.close() verify(integrationMock).close() } @Test fun `When defaultUncaughtExceptionHandler is disabled, should not install Sentry UncaughtExceptionHandler`() { - val options = SentryOptions() - options.isEnableUncaughtExceptionHandler = false - val hub = mock() - val handlerMock = mock() - val integration = UncaughtExceptionHandlerIntegration(handlerMock) - integration.register(hub, options) - verify(handlerMock, never()).defaultUncaughtExceptionHandler = any() + val sut = fixture.getSut( + enableUncaughtExceptionHandler = false, + isPrintUncaughtStackTrace = false + ) + + sut.register(fixture.hub, fixture.options) + + verify(fixture.handler, never()).defaultUncaughtExceptionHandler = any() } @Test fun `When defaultUncaughtExceptionHandler is enabled, should install Sentry UncaughtExceptionHandler`() { - val options = SentryOptions() - val hub = mock() - val handlerMock = mock() - val integration = UncaughtExceptionHandlerIntegration(handlerMock) - integration.register(hub, options) - verify(handlerMock).defaultUncaughtExceptionHandler = argWhere { it is UncaughtExceptionHandlerIntegration } + val sut = fixture.getSut(isPrintUncaughtStackTrace = false) + + sut.register(fixture.hub, fixture.options) + + verify(fixture.handler).defaultUncaughtExceptionHandler = + argWhere { it is UncaughtExceptionHandlerIntegration } } @Test fun `When defaultUncaughtExceptionHandler is set and integration is closed, default uncaught exception handler is reset to previous handler`() { - val handlerMock = mock() - val integration = UncaughtExceptionHandlerIntegration(handlerMock) - - val defaultExceptionHandler = mock() - whenever(handlerMock.defaultUncaughtExceptionHandler) - .thenReturn(defaultExceptionHandler) - integration.register(mock(), SentryOptions()) - whenever(handlerMock.defaultUncaughtExceptionHandler) - .thenReturn(integration) - integration.close() - verify(handlerMock).defaultUncaughtExceptionHandler = defaultExceptionHandler + val sut = fixture.getSut(hasDefaultHandler = true, isPrintUncaughtStackTrace = false) + + sut.register(fixture.hub, fixture.options) + whenever(fixture.handler.defaultUncaughtExceptionHandler) + .thenReturn(sut) + sut.close() + + verify(fixture.handler).defaultUncaughtExceptionHandler = fixture.defaultHandler } @Test fun `When defaultUncaughtExceptionHandler is not set and integration is closed, default uncaught exception handler is reset to null`() { - val handlerMock = mock() - val integration = UncaughtExceptionHandlerIntegration(handlerMock) - - whenever(handlerMock.defaultUncaughtExceptionHandler) - .thenReturn(null) - integration.register(mock(), SentryOptions()) - whenever(handlerMock.defaultUncaughtExceptionHandler) - .thenReturn(integration) - integration.close() - verify(handlerMock).defaultUncaughtExceptionHandler = null + val sut = fixture.getSut(isPrintUncaughtStackTrace = false) + + sut.register(fixture.hub, fixture.options) + whenever(fixture.handler.defaultUncaughtExceptionHandler) + .thenReturn(sut) + sut.close() + + verify(fixture.handler).defaultUncaughtExceptionHandler = null } @Test @@ -160,12 +160,10 @@ class UncaughtExceptionHandlerIntegrationTest { val outputStreamCaptor = ByteArrayOutputStream() System.setErr(PrintStream(outputStreamCaptor)) - val handlerMock = mock() - val options = SentryOptions().noFlushTimeout() - options.isPrintUncaughtStackTrace = true - val sut = UncaughtExceptionHandlerIntegration(handlerMock) - sut.register(mock(), options) - sut.uncaughtException(mock(), RuntimeException("This should be printed!")) + val sut = fixture.getSut(isPrintUncaughtStackTrace = true) + + sut.register(fixture.hub, fixture.options) + sut.uncaughtException(fixture.thread, RuntimeException("This should be printed!")) assertTrue( outputStreamCaptor.toString() @@ -179,4 +177,51 @@ class UncaughtExceptionHandlerIntegrationTest { System.setErr(standardErr) } } + + @Test + fun `waits for event to flush on disk`() { + val capturedEventId = SentryId() + + whenever(fixture.hub.captureEvent(any(), any())).thenAnswer { invocation -> + val hint = HintUtils.getSentrySdkHint(invocation.getArgument(1)) + as DiskFlushNotification + thread { + Thread.sleep(1000L) + hint.markFlushed() + } + capturedEventId + } + + val sut = fixture.getSut(flushTimeoutMillis = 5000) + + sut.register(fixture.hub, fixture.options) + sut.uncaughtException(fixture.thread, fixture.throwable) + + verify(fixture.hub).captureEvent(any(), any()) + // shouldn't fall into timed out state, because we marked event as flushed on another thread + verify(fixture.logger, never()).log( + any(), + argThat { startsWith("Timed out") }, + any() + ) + } + + @Test + fun `does not block flushing when the event was dropped`() { + whenever(fixture.hub.captureEvent(any(), any())).thenReturn(SentryId.EMPTY_ID) + + val sut = fixture.getSut() + + sut.register(fixture.hub, fixture.options) + sut.uncaughtException(fixture.thread, fixture.throwable) + + verify(fixture.hub).captureEvent(any(), any()) + // we do not call markFlushed, hence it should time out waiting for flush, but because + // we drop the event, it should not even come to this if-check + verify(fixture.logger, never()).log( + any(), + argThat { startsWith("Timed out") }, + any() + ) + } }