From f33a11d0bdd89fbc509ca5f1476903b31594b24e Mon Sep 17 00:00:00 2001 From: Stefano Date: Fri, 16 Feb 2024 11:48:07 +0100 Subject: [PATCH] Don't wait on main thread when SDK restarts (#3200) * added new methods to close instances with isRestarting flag: - IHub.close(Boolean) - ISentryClient.close(Boolean) - ITransport.close(Boolean) * when the SDK restarts, the executor service is now closed in the background, and current network connections are dropped * AsyncHttpTransport now stores the current runnable in a variable and runs it in the rejectedExecutionHandler on close --- CHANGELOG.md | 3 +- .../android/core/InternalSentrySdkTest.kt | 4 + .../core/SessionTrackingIntegrationTest.kt | 4 + .../io/sentry/uitest/android/EnvelopeTests.kt | 2 - .../io/sentry/uitest/android/SdkInitTests.kt | 80 +++++++++++++++++++ .../uitest/android/mockservers/MockRelay.kt | 28 ++++--- .../android/mockservers/RelayAsserter.kt | 8 +- .../api/sentry-apache-http-client-5.api | 1 + .../apache/ApacheHttpClientTransport.java | 7 +- .../apache/ApacheHttpClientTransportTest.kt | 17 +++- sentry/api/sentry.api | 10 +++ sentry/src/main/java/io/sentry/Hub.java | 15 +++- .../src/main/java/io/sentry/HubAdapter.java | 5 ++ sentry/src/main/java/io/sentry/IHub.java | 7 ++ .../main/java/io/sentry/ISentryClient.java | 7 ++ sentry/src/main/java/io/sentry/NoOpHub.java | 3 + .../main/java/io/sentry/NoOpSentryClient.java | 3 + sentry/src/main/java/io/sentry/Sentry.java | 4 +- .../src/main/java/io/sentry/SentryClient.java | 9 ++- .../sentry/transport/AsyncHttpTransport.java | 21 ++++- .../java/io/sentry/transport/ITransport.java | 7 ++ .../io/sentry/transport/NoOpTransport.java | 3 + .../io/sentry/transport/StdoutTransport.java | 3 + .../src/test/java/io/sentry/HubAdapterTest.kt | 12 ++- sentry/src/test/java/io/sentry/HubTest.kt | 47 ++++++++++- sentry/src/test/java/io/sentry/NoOpHubTest.kt | 12 +++ .../java/io/sentry/NoOpSentryClientTest.kt | 12 +++ .../test/java/io/sentry/SentryClientTest.kt | 19 +++++ sentry/src/test/java/io/sentry/SentryTest.kt | 24 ++++++ .../transport/AsyncHttpTransportTest.kt | 35 ++++++++ 30 files changed, 385 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index afb4f439c3..71f47af9d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,8 +7,9 @@ - Add new threshold parameters to monitor config ([#3181](https://github.com/getsentry/sentry-java/pull/3181)) - Report process init time as a span for app start performance ([#3159](https://github.com/getsentry/sentry-java/pull/3159)) -## Fixes +### Fixes +- Don't wait on main thread when SDK restarts ([#3200](https://github.com/getsentry/sentry-java/pull/3200)) - Fix Jetpack Compose widgets are not being correctly identified for user interaction tracing ([#3209](https://github.com/getsentry/sentry-java/pull/3209)) ## 7.3.0 diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/InternalSentrySdkTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/InternalSentrySdkTest.kt index 766ee95f5c..a10d8add7b 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/InternalSentrySdkTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/InternalSentrySdkTest.kt @@ -54,6 +54,10 @@ class InternalSentrySdkTest { options.dsn = "https://key@host/proj" options.setTransportFactory { _, _ -> object : ITransport { + override fun close(isRestarting: Boolean) { + // no-op + } + override fun close() { // no-op } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/SessionTrackingIntegrationTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/SessionTrackingIntegrationTest.kt index af32fa3714..3bb853d271 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/SessionTrackingIntegrationTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/SessionTrackingIntegrationTest.kt @@ -133,6 +133,10 @@ class SessionTrackingIntegrationTest { TODO("Not yet implemented") } + override fun close(isRestarting: Boolean) { + TODO("Not yet implemented") + } + override fun close() { TODO("Not yet implemented") } diff --git a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt index dab6c1eb60..6825933ea3 100644 --- a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt +++ b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt @@ -1,6 +1,5 @@ package io.sentry.uitest.android -import android.util.Log import androidx.lifecycle.Lifecycle import androidx.test.core.app.launchActivity import androidx.test.espresso.Espresso @@ -199,7 +198,6 @@ class EnvelopeTests : BaseUiTest() { relay.assert { findEnvelope { - Log.e("ITEMS", it.items.joinToString { item -> item.header.type.itemType }) assertEnvelopeTransaction(it.items.toList(), AndroidLogger()).transaction == "timedOutProfile" }.assert { val transactionItem: SentryTransaction = it.assertTransaction() diff --git a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/SdkInitTests.kt b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/SdkInitTests.kt index 89c269eaff..c942783548 100644 --- a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/SdkInitTests.kt +++ b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/SdkInitTests.kt @@ -12,6 +12,7 @@ import io.sentry.protocol.SentryTransaction import org.junit.runner.RunWith import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertTrue @RunWith(AndroidJUnit4::class) class SdkInitTests : BaseUiTest() { @@ -74,4 +75,83 @@ class SdkInitTests : BaseUiTest() { assertNoOtherEnvelopes() } } + + @Test + fun doubleInitDoesNotWait() { + relayIdlingResource.increment() + // Let's make the first request timeout + relay.addTimeoutResponse() + + initSentry(true) { options: SentryAndroidOptions -> + options.tracesSampleRate = 1.0 + } + + Sentry.startTransaction("beforeRestart", "emptyTransaction").finish() + + // We want the SDK to start sending the event. If we don't wait, it's possible we don't send anything before the SDK is restarted + waitUntilIdle() + + relayIdlingResource.increment() + relayIdlingResource.increment() + + val beforeRestart = System.currentTimeMillis() + // We restart the SDK. This shouldn't block the main thread, but new options (e.g. profiling) should work + initSentry(true) { options: SentryAndroidOptions -> + options.tracesSampleRate = 1.0 + options.profilesSampleRate = 1.0 + } + val afterRestart = System.currentTimeMillis() + val restartMs = afterRestart - beforeRestart + + Sentry.startTransaction("afterRestart", "emptyTransaction").finish() + // We assert for less than 1 second just to account for slow devices in saucelabs or headless emulator + assertTrue(restartMs < 1000, "Expected less than 1000 ms for SDK restart. Got $restartMs ms") + + relay.assert { + findEnvelope { + assertEnvelopeTransaction(it.items.toList()).transaction == "beforeRestart" + }.assert { + it.assertTransaction() + // No profiling item, as in the first init it was not enabled + it.assertNoOtherItems() + } + findEnvelope { + assertEnvelopeTransaction(it.items.toList()).transaction == "afterRestart" + }.assert { + it.assertTransaction() + // There is a profiling item, as in the second init it was enabled + it.assertProfile() + it.assertNoOtherItems() + } + assertNoOtherEnvelopes() + } + } + + @Test + fun initCloseInitWaits() { + relayIdlingResource.increment() + // Let's make the first request timeout + relay.addTimeoutResponse() + + initSentry(true) { options: SentryAndroidOptions -> + options.tracesSampleRate = 1.0 + options.flushTimeoutMillis = 3000 + } + + Sentry.startTransaction("beforeRestart", "emptyTransaction").finish() + + // We want the SDK to start sending the event. If we don't wait, it's possible we don't send anything before the SDK is restarted + waitUntilIdle() + + val beforeRestart = System.currentTimeMillis() + Sentry.close() + // We stop the SDK. This should block the main thread. Then we start it again with new options + initSentry(true) { options: SentryAndroidOptions -> + options.tracesSampleRate = 1.0 + options.profilesSampleRate = 1.0 + } + val afterRestart = System.currentTimeMillis() + val restartMs = afterRestart - beforeRestart + assertTrue(restartMs > 3000, "Expected more than 3000 ms for SDK close and restart. Got $restartMs ms") + } } diff --git a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/mockservers/MockRelay.kt b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/mockservers/MockRelay.kt index 86e032e007..9074e1e2a5 100644 --- a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/mockservers/MockRelay.kt +++ b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/mockservers/MockRelay.kt @@ -6,6 +6,7 @@ import okhttp3.mockwebserver.Dispatcher import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer import okhttp3.mockwebserver.RecordedRequest +import okhttp3.mockwebserver.SocketPolicy import kotlin.test.assertNotNull /** Mocks a relay server. */ @@ -32,24 +33,24 @@ class MockRelay( init { relay.dispatcher = object : Dispatcher() { override fun dispatch(request: RecordedRequest): MockResponse { - // If a request with a body size of 0 is received, we drop it. - // This shouldn't happen in reality, but it rarely happens in tests. - if (request.bodySize == 0L || request.failure != null) { - return MockResponse() - } // We check if there is any custom response previously set to return to this request, // otherwise we return a successful MockResponse. - val response = responses.asSequence() - .mapNotNull { it(request) } - .firstOrNull() - ?: MockResponse() + val response = responses.removeFirstOrNull()?.let { it(request) } ?: MockResponse() // We should receive only envelopes on this path. if (request.path == envelopePath) { val relayResponse = RelayAsserter.RelayResponse(request, response) + // If we reply with NO_RESPONSE, we can ignore the request, so we can return here + if (relayResponse.envelope == null || response.socketPolicy == SocketPolicy.NO_RESPONSE) { + // If we are waiting for requests to be received, we decrement the associated counter. + if (waitForRequests) { + relayIdlingResource.decrement() + } + return response + } assertNotNull(relayResponse.envelope) val envelopeId: String = relayResponse.envelope!!.header.eventId!!.toString() - // If we already received the envelope (e.g. retrying mechanism) we drop it + // If we already received the envelope (e.g. retrying mechanism) we ignore it if (receivedEnvelopes.contains(envelopeId)) { return MockResponse() } @@ -90,6 +91,13 @@ class MockRelay( responses.add(0, response) } + /** Add a custom response to be returned at the next request received. */ + fun addTimeoutResponse() { + addResponse { + MockResponse().setSocketPolicy(SocketPolicy.NO_RESPONSE) + } + } + /** Add a custom response to be returned at the next request received, if it satisfies the [filter]. */ fun addResponse( filter: (RecordedRequest) -> Boolean, diff --git a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/mockservers/RelayAsserter.kt b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/mockservers/RelayAsserter.kt index 064bf724db..e956978086 100644 --- a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/mockservers/RelayAsserter.kt +++ b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/mockservers/RelayAsserter.kt @@ -40,7 +40,13 @@ class RelayAsserter( filter: (envelope: SentryEnvelope) -> Boolean = { true } ): RelayResponse { val relayResponseIndex = unassertedEnvelopes.indexOfFirst { it.envelope?.let(filter) ?: false } - if (relayResponseIndex == -1) throw AssertionError("No envelope request found with specified filter") + if (relayResponseIndex == -1) { + throw AssertionError( + "No envelope request found with specified filter.\n" + + "There was a total of ${originalUnassertedEnvelopes.size} envelopes: " + + originalUnassertedEnvelopes.joinToString { describeEnvelope(it.envelope!!) } + ) + } return unassertedEnvelopes.removeAt(relayResponseIndex) } diff --git a/sentry-apache-http-client-5/api/sentry-apache-http-client-5.api b/sentry-apache-http-client-5/api/sentry-apache-http-client-5.api index 9f33a4f115..37843521a5 100644 --- a/sentry-apache-http-client-5/api/sentry-apache-http-client-5.api +++ b/sentry-apache-http-client-5/api/sentry-apache-http-client-5.api @@ -1,6 +1,7 @@ public final class io/sentry/transport/apache/ApacheHttpClientTransport : io/sentry/transport/ITransport { public fun (Lio/sentry/SentryOptions;Lio/sentry/RequestDetails;Lorg/apache/hc/client5/http/impl/async/CloseableHttpAsyncClient;Lio/sentry/transport/RateLimiter;)V public fun close ()V + public fun close (Z)V public fun flush (J)V public fun getRateLimiter ()Lio/sentry/transport/RateLimiter; public fun send (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V diff --git a/sentry-apache-http-client-5/src/main/java/io/sentry/transport/apache/ApacheHttpClientTransport.java b/sentry-apache-http-client-5/src/main/java/io/sentry/transport/apache/ApacheHttpClientTransport.java index 2dfa1fdcda..2cf1484b56 100644 --- a/sentry-apache-http-client-5/src/main/java/io/sentry/transport/apache/ApacheHttpClientTransport.java +++ b/sentry-apache-http-client-5/src/main/java/io/sentry/transport/apache/ApacheHttpClientTransport.java @@ -196,9 +196,14 @@ public void flush(long timeoutMillis) { @Override public void close() throws IOException { + close(false); + } + + @Override + public void close(final boolean isRestarting) throws IOException { options.getLogger().log(DEBUG, "Shutting down"); try { - httpclient.awaitShutdown(TimeValue.ofSeconds(1)); + httpclient.awaitShutdown(TimeValue.ofSeconds(isRestarting ? 0 : 1)); } catch (InterruptedException e) { options.getLogger().log(DEBUG, "Thread interrupted while closing the connection."); Thread.currentThread().interrupt(); diff --git a/sentry-apache-http-client-5/src/test/kotlin/io/sentry/transport/apache/ApacheHttpClientTransportTest.kt b/sentry-apache-http-client-5/src/test/kotlin/io/sentry/transport/apache/ApacheHttpClientTransportTest.kt index 1fe02f7d2c..465aefc70d 100644 --- a/sentry-apache-http-client-5/src/test/kotlin/io/sentry/transport/apache/ApacheHttpClientTransportTest.kt +++ b/sentry-apache-http-client-5/src/test/kotlin/io/sentry/transport/apache/ApacheHttpClientTransportTest.kt @@ -31,6 +31,7 @@ import java.util.concurrent.Executors import kotlin.test.AfterTest import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertNotEquals class ApacheHttpClientTransportTest { @@ -116,7 +117,21 @@ class ApacheHttpClientTransportTest { fun `close waits for shutdown`() { val sut = fixture.getSut() sut.close() - verify(fixture.client).awaitShutdown(any()) + verify(fixture.client).awaitShutdown(check { assertNotEquals(0L, it.duration) }) + } + + @Test + fun `close with isRestarting false waits for shutdown`() { + val sut = fixture.getSut() + sut.close(false) + verify(fixture.client).awaitShutdown(check { assertNotEquals(0L, it.duration) }) + } + + @Test + fun `close with isRestarting true does not wait for shutdown`() { + val sut = fixture.getSut() + sut.close(true) + verify(fixture.client).awaitShutdown(check { assertEquals(0L, it.duration) }) } @Test diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 03cd96ad0d..3a953c1587 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -427,6 +427,7 @@ public final class io/sentry/Hub : io/sentry/IHub { public fun clone ()Lio/sentry/IHub; public synthetic fun clone ()Ljava/lang/Object; public fun close ()V + public fun close (Z)V public fun configureScope (Lio/sentry/ScopeCallback;)V public fun continueTrace (Ljava/lang/String;Ljava/util/List;)Lio/sentry/TransactionContext; public fun endSession ()V @@ -477,6 +478,7 @@ public final class io/sentry/HubAdapter : io/sentry/IHub { public fun clone ()Lio/sentry/IHub; public synthetic fun clone ()Ljava/lang/Object; public fun close ()V + public fun close (Z)V public fun configureScope (Lio/sentry/ScopeCallback;)V public fun continueTrace (Ljava/lang/String;Ljava/util/List;)Lio/sentry/TransactionContext; public fun endSession ()V @@ -567,6 +569,7 @@ public abstract interface class io/sentry/IHub { public abstract fun clearBreadcrumbs ()V public abstract fun clone ()Lio/sentry/IHub; public abstract fun close ()V + public abstract fun close (Z)V public abstract fun configureScope (Lio/sentry/ScopeCallback;)V public abstract fun continueTrace (Ljava/lang/String;Ljava/util/List;)Lio/sentry/TransactionContext; public abstract fun endSession ()V @@ -732,6 +735,7 @@ public abstract interface class io/sentry/ISentryClient { public abstract fun captureTransaction (Lio/sentry/protocol/SentryTransaction;Lio/sentry/TraceContext;Lio/sentry/IScope;Lio/sentry/Hint;Lio/sentry/ProfilingTraceData;)Lio/sentry/protocol/SentryId; public abstract fun captureUserFeedback (Lio/sentry/UserFeedback;)V public abstract fun close ()V + public abstract fun close (Z)V public abstract fun flush (J)V public abstract fun getRateLimiter ()Lio/sentry/transport/RateLimiter; public abstract fun isEnabled ()Z @@ -1131,6 +1135,7 @@ public final class io/sentry/NoOpHub : io/sentry/IHub { public fun clone ()Lio/sentry/IHub; public synthetic fun clone ()Ljava/lang/Object; public fun close ()V + public fun close (Z)V public fun configureScope (Lio/sentry/ScopeCallback;)V public fun continueTrace (Ljava/lang/String;Ljava/util/List;)Lio/sentry/TransactionContext; public fun endSession ()V @@ -1849,6 +1854,7 @@ public final class io/sentry/SentryClient : io/sentry/ISentryClient { public fun captureTransaction (Lio/sentry/protocol/SentryTransaction;Lio/sentry/TraceContext;Lio/sentry/IScope;Lio/sentry/Hint;Lio/sentry/ProfilingTraceData;)Lio/sentry/protocol/SentryId; public fun captureUserFeedback (Lio/sentry/UserFeedback;)V public fun close ()V + public fun close (Z)V public fun flush (J)V public fun getRateLimiter ()Lio/sentry/transport/RateLimiter; public fun isEnabled ()Z @@ -4536,6 +4542,7 @@ public final class io/sentry/transport/AsyncHttpTransport : io/sentry/transport/ public fun (Lio/sentry/SentryOptions;Lio/sentry/transport/RateLimiter;Lio/sentry/transport/ITransportGate;Lio/sentry/RequestDetails;)V public fun (Lio/sentry/transport/QueuedThreadPoolExecutor;Lio/sentry/SentryOptions;Lio/sentry/transport/RateLimiter;Lio/sentry/transport/ITransportGate;Lio/sentry/transport/HttpConnection;)V public fun close ()V + public fun close (Z)V public fun flush (J)V public fun getRateLimiter ()Lio/sentry/transport/RateLimiter; public fun isHealthy ()Z @@ -4552,6 +4559,7 @@ public abstract interface class io/sentry/transport/ICurrentDateProvider { } public abstract interface class io/sentry/transport/ITransport : java/io/Closeable { + public abstract fun close (Z)V public abstract fun flush (J)V public abstract fun getRateLimiter ()Lio/sentry/transport/RateLimiter; public fun isHealthy ()Z @@ -4573,6 +4581,7 @@ public final class io/sentry/transport/NoOpEnvelopeCache : io/sentry/cache/IEnve public final class io/sentry/transport/NoOpTransport : io/sentry/transport/ITransport { public fun close ()V + public fun close (Z)V public fun flush (J)V public static fun getInstance ()Lio/sentry/transport/NoOpTransport; public fun getRateLimiter ()Lio/sentry/transport/RateLimiter; @@ -4606,6 +4615,7 @@ public final class io/sentry/transport/ReusableCountLatch { public final class io/sentry/transport/StdoutTransport : io/sentry/transport/ITransport { public fun (Lio/sentry/ISerializer;)V public fun close ()V + public fun close (Z)V public fun flush (J)V public fun getRateLimiter ()Lio/sentry/transport/RateLimiter; public fun send (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V diff --git a/sentry/src/main/java/io/sentry/Hub.java b/sentry/src/main/java/io/sentry/Hub.java index e7942722fc..b2e998e7f4 100644 --- a/sentry/src/main/java/io/sentry/Hub.java +++ b/sentry/src/main/java/io/sentry/Hub.java @@ -331,6 +331,12 @@ public void endSession() { @Override public void close() { + close(false); + } + + @Override + @SuppressWarnings("FutureReturnValueIgnored") + public void close(final boolean isRestarting) { if (!isEnabled()) { options .getLogger() @@ -352,12 +358,17 @@ public void close() { configureScope(scope -> scope.clear()); options.getTransactionProfiler().close(); options.getTransactionPerformanceCollector().close(); - options.getExecutorService().close(options.getShutdownTimeoutMillis()); + final @NotNull ISentryExecutorService executorService = options.getExecutorService(); + if (isRestarting) { + executorService.submit(() -> executorService.close(options.getShutdownTimeoutMillis())); + } else { + executorService.close(options.getShutdownTimeoutMillis()); + } // Close the top-most client final StackItem item = stack.peek(); // TODO: should we end session before closing client? - item.getClient().close(); + item.getClient().close(isRestarting); } catch (Throwable e) { options.getLogger().log(SentryLevel.ERROR, "Error while closing the Hub.", e); } diff --git a/sentry/src/main/java/io/sentry/HubAdapter.java b/sentry/src/main/java/io/sentry/HubAdapter.java index 68a9bdf11d..6ce1281895 100644 --- a/sentry/src/main/java/io/sentry/HubAdapter.java +++ b/sentry/src/main/java/io/sentry/HubAdapter.java @@ -78,6 +78,11 @@ public void endSession() { Sentry.endSession(); } + @Override + public void close(final boolean isRestarting) { + Sentry.close(); + } + @Override public void close() { Sentry.close(); diff --git a/sentry/src/main/java/io/sentry/IHub.java b/sentry/src/main/java/io/sentry/IHub.java index 01431043f0..92e2d6e1f9 100644 --- a/sentry/src/main/java/io/sentry/IHub.java +++ b/sentry/src/main/java/io/sentry/IHub.java @@ -191,6 +191,13 @@ SentryId captureException( /** Flushes out the queue for up to timeout seconds and disable the Hub. */ void close(); + /** + * Flushes out the queue for up to timeout seconds and disable the Hub. + * + * @param isRestarting if true, avoids locking the main thread when finishing the queue. + */ + void close(boolean isRestarting); + /** * Adds a breadcrumb to the current Scope * diff --git a/sentry/src/main/java/io/sentry/ISentryClient.java b/sentry/src/main/java/io/sentry/ISentryClient.java index 15b5f25c4b..31b432c560 100644 --- a/sentry/src/main/java/io/sentry/ISentryClient.java +++ b/sentry/src/main/java/io/sentry/ISentryClient.java @@ -32,6 +32,13 @@ public interface ISentryClient { /** Flushes out the queue for up to timeout seconds and disable the client. */ void close(); + /** + * Flushes out the queue for up to timeout seconds and disable the client. + * + * @param isRestarting if true, avoids locking the main thread when finishing the queue. + */ + void close(boolean isRestarting); + /** * Flushes events queued up, but keeps the client enabled. Not implemented yet. * diff --git a/sentry/src/main/java/io/sentry/NoOpHub.java b/sentry/src/main/java/io/sentry/NoOpHub.java index d186c69ca2..e72a59e5f5 100644 --- a/sentry/src/main/java/io/sentry/NoOpHub.java +++ b/sentry/src/main/java/io/sentry/NoOpHub.java @@ -76,6 +76,9 @@ public void endSession() {} @Override public void close() {} + @Override + public void close(final boolean isRestarting) {} + @Override public void addBreadcrumb(@NotNull Breadcrumb breadcrumb, @Nullable Hint hint) {} diff --git a/sentry/src/main/java/io/sentry/NoOpSentryClient.java b/sentry/src/main/java/io/sentry/NoOpSentryClient.java index a37d09eb89..e2ab32c50a 100644 --- a/sentry/src/main/java/io/sentry/NoOpSentryClient.java +++ b/sentry/src/main/java/io/sentry/NoOpSentryClient.java @@ -28,6 +28,9 @@ public boolean isEnabled() { return SentryId.EMPTY_ID; } + @Override + public void close(final boolean isRestarting) {} + @Override public void close() {} diff --git a/sentry/src/main/java/io/sentry/Sentry.java b/sentry/src/main/java/io/sentry/Sentry.java index 0aff89c0d0..deb01f72f9 100644 --- a/sentry/src/main/java/io/sentry/Sentry.java +++ b/sentry/src/main/java/io/sentry/Sentry.java @@ -237,7 +237,7 @@ private static synchronized void init( currentHub.set(mainHub); - hub.close(); + hub.close(true); // If the executorService passed in the init is the same that was previously closed, we have to // set a new one @@ -484,7 +484,7 @@ public static synchronized void close() { mainHub = NoOpHub.getInstance(); // remove thread local to avoid memory leak currentHub.remove(); - hub.close(); + hub.close(false); } /** diff --git a/sentry/src/main/java/io/sentry/SentryClient.java b/sentry/src/main/java/io/sentry/SentryClient.java index 2973c1f8bc..57f4559ab3 100644 --- a/sentry/src/main/java/io/sentry/SentryClient.java +++ b/sentry/src/main/java/io/sentry/SentryClient.java @@ -903,11 +903,16 @@ private void sortBreadcrumbsByDate( @Override public void close() { + close(false); + } + + @Override + public void close(final boolean isRestarting) { options.getLogger().log(SentryLevel.INFO, "Closing SentryClient."); try { - flush(options.getShutdownTimeoutMillis()); - transport.close(); + flush(isRestarting ? 0 : options.getShutdownTimeoutMillis()); + transport.close(isRestarting); } catch (IOException e) { options .getLogger() diff --git a/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java b/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java index 6636ce517f..985bbcccbb 100644 --- a/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java +++ b/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java @@ -26,6 +26,7 @@ import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeUnit; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; /** * {@link ITransport} implementation that executes request asynchronously in a blocking manner using @@ -39,6 +40,7 @@ public final class AsyncHttpTransport implements ITransport { private final @NotNull RateLimiter rateLimiter; private final @NotNull ITransportGate transportGate; private final @NotNull HttpConnection connection; + private volatile @Nullable Runnable currentRunnable = null; public AsyncHttpTransport( final @NotNull SentryOptions options, @@ -163,16 +165,29 @@ public boolean isHealthy() { @Override public void close() throws IOException { + close(false); + } + + @Override + public void close(final boolean isRestarting) throws IOException { executor.shutdown(); options.getLogger().log(SentryLevel.DEBUG, "Shutting down"); try { - if (!executor.awaitTermination(options.getFlushTimeoutMillis(), TimeUnit.MILLISECONDS)) { + // We need a small timeout to be able to save to disk any rejected envelope + long timeout = isRestarting ? 0 : options.getFlushTimeoutMillis(); + if (!executor.awaitTermination(timeout, TimeUnit.MILLISECONDS)) { options .getLogger() .log( SentryLevel.WARNING, - "Failed to shutdown the async connection async sender within 1 minute. Trying to force it now."); + "Failed to shutdown the async connection async sender within " + + timeout + + " ms. Trying to force it now."); executor.shutdownNow(); + if (currentRunnable != null) { + // We store to disk any envelope that is currently being sent + executor.getRejectedExecutionHandler().rejectedExecution(currentRunnable, executor); + } } } catch (InterruptedException e) { // ok, just give up then... @@ -222,6 +237,7 @@ private final class EnvelopeSender implements Runnable { @Override public void run() { + currentRunnable = this; TransportResult result = this.failedResult; try { result = flush(); @@ -243,6 +259,7 @@ public void run() { finalResult.isSuccess()); submissionResult.setResult(finalResult.isSuccess()); }); + currentRunnable = null; } } diff --git a/sentry/src/main/java/io/sentry/transport/ITransport.java b/sentry/src/main/java/io/sentry/transport/ITransport.java index b7a38d20ed..ccc3db4a0c 100644 --- a/sentry/src/main/java/io/sentry/transport/ITransport.java +++ b/sentry/src/main/java/io/sentry/transport/ITransport.java @@ -28,4 +28,11 @@ default boolean isHealthy() { @Nullable RateLimiter getRateLimiter(); + + /** + * Closes the transport. + * + * @param isRestarting if true, avoids locking the main thread by dropping the current connection. + */ + void close(boolean isRestarting) throws IOException; } diff --git a/sentry/src/main/java/io/sentry/transport/NoOpTransport.java b/sentry/src/main/java/io/sentry/transport/NoOpTransport.java index f73605b049..6292d4efeb 100644 --- a/sentry/src/main/java/io/sentry/transport/NoOpTransport.java +++ b/sentry/src/main/java/io/sentry/transport/NoOpTransport.java @@ -32,4 +32,7 @@ public void flush(long timeoutMillis) {} @Override public void close() throws IOException {} + + @Override + public void close(final boolean isRestarting) throws IOException {} } diff --git a/sentry/src/main/java/io/sentry/transport/StdoutTransport.java b/sentry/src/main/java/io/sentry/transport/StdoutTransport.java index 99aed10eac..c24f96dbff 100644 --- a/sentry/src/main/java/io/sentry/transport/StdoutTransport.java +++ b/sentry/src/main/java/io/sentry/transport/StdoutTransport.java @@ -41,4 +41,7 @@ public void flush(long timeoutMillis) { @Override public void close() {} + + @Override + public void close(final boolean isRestarting) {} } diff --git a/sentry/src/test/java/io/sentry/HubAdapterTest.kt b/sentry/src/test/java/io/sentry/HubAdapterTest.kt index df4c0bdbcc..9686250d20 100644 --- a/sentry/src/test/java/io/sentry/HubAdapterTest.kt +++ b/sentry/src/test/java/io/sentry/HubAdapterTest.kt @@ -93,7 +93,17 @@ class HubAdapterTest { @Test fun `close calls Hub`() { HubAdapter.getInstance().close() - verify(hub).close() + verify(hub).close(false) + } + + @Test fun `close with isRestarting true calls Hub with isRestarting false`() { + HubAdapter.getInstance().close(true) + verify(hub).close(false) + } + + @Test fun `close with isRestarting false calls Hub with isRestarting false`() { + HubAdapter.getInstance().close(false) + verify(hub).close(false) } @Test fun `addBreadcrumb calls Hub`() { diff --git a/sentry/src/test/java/io/sentry/HubTest.kt b/sentry/src/test/java/io/sentry/HubTest.kt index 2341ce8ad1..d6afaa405b 100644 --- a/sentry/src/test/java/io/sentry/HubTest.kt +++ b/sentry/src/test/java/io/sentry/HubTest.kt @@ -10,6 +10,7 @@ import io.sentry.hints.SessionStartHint import io.sentry.protocol.SentryId import io.sentry.protocol.SentryTransaction import io.sentry.protocol.User +import io.sentry.test.DeferredExecutorService import io.sentry.test.callMethod import io.sentry.util.HintUtils import io.sentry.util.StringUtils @@ -774,7 +775,7 @@ class HubTest { sut.close() sut.close() - verify(mockClient).close() // 1 to close, but next one wont be recorded + verify(mockClient).close(eq(false)) // 1 to close, but next one wont be recorded } @Test @@ -782,7 +783,23 @@ class HubTest { val (sut, mockClient) = getEnabledHub() sut.close() - verify(mockClient).close() + verify(mockClient).close(eq(false)) + } + + @Test + fun `when close is called with isRestarting false and client is alive, close on the client should be called with isRestarting false`() { + val (sut, mockClient) = getEnabledHub() + + sut.close(false) + verify(mockClient).close(eq(false)) + } + + @Test + fun `when close is called with isRestarting true and client is alive, close on the client should be called with isRestarting true`() { + val (sut, mockClient) = getEnabledHub() + + sut.close(true) + verify(mockClient).close(eq(true)) } //endregion @@ -1652,6 +1669,32 @@ class HubTest { verify(performanceCollector).close() } + @Test + fun `Hub with isRestarting true should close the sentry executor in the background`() { + val executor = spy(DeferredExecutorService()) + val options = SentryOptions().apply { + dsn = "https://key@sentry.io/proj" + executorService = executor + } + val sut = Hub(options) + sut.close(true) + verify(executor, never()).close(any()) + executor.runAll() + verify(executor).close(any()) + } + + @Test + fun `Hub with isRestarting false should close the sentry executor in the background`() { + val executor = mock() + val options = SentryOptions().apply { + dsn = "https://key@sentry.io/proj" + executorService = executor + } + val sut = Hub(options) + sut.close(false) + verify(executor).close(any()) + } + @Test fun `Hub close should clear the scope`() { val options = SentryOptions().apply { diff --git a/sentry/src/test/java/io/sentry/NoOpHubTest.kt b/sentry/src/test/java/io/sentry/NoOpHubTest.kt index 868ebaa81e..dbbfb4b4f1 100644 --- a/sentry/src/test/java/io/sentry/NoOpHubTest.kt +++ b/sentry/src/test/java/io/sentry/NoOpHubTest.kt @@ -46,6 +46,18 @@ class NoOpHubTest { assertEquals(SentryId.EMPTY_ID, sut.captureEvent(SentryEvent())) } + @Test + fun `close with isRestarting true does not affect captureEvent`() { + sut.close(true) + assertEquals(SentryId.EMPTY_ID, sut.captureEvent(SentryEvent())) + } + + @Test + fun `close with isRestarting false does not affect captureEvent`() { + sut.close(false) + assertEquals(SentryId.EMPTY_ID, sut.captureEvent(SentryEvent())) + } + @Test fun `close does not affect captureException`() { sut.close() diff --git a/sentry/src/test/java/io/sentry/NoOpSentryClientTest.kt b/sentry/src/test/java/io/sentry/NoOpSentryClientTest.kt index 67264095ff..919ce5f083 100644 --- a/sentry/src/test/java/io/sentry/NoOpSentryClientTest.kt +++ b/sentry/src/test/java/io/sentry/NoOpSentryClientTest.kt @@ -35,6 +35,18 @@ class NoOpSentryClientTest { assertEquals(SentryId.EMPTY_ID, sut.callMethod("captureEvent", SentryEvent::class.java, null)) } + @Test + fun `close with isRestarting true does not affect captureEvent`() { + sut.close(true) + assertEquals(SentryId.EMPTY_ID, sut.callMethod("captureEvent", SentryEvent::class.java, null)) + } + + @Test + fun `close with isRestarting false does not affect captureEvent`() { + sut.close(false) + assertEquals(SentryId.EMPTY_ID, sut.callMethod("captureEvent", SentryEvent::class.java, null)) + } + @Test fun `close does not affect captureException`() { sut.close() diff --git a/sentry/src/test/java/io/sentry/SentryClientTest.kt b/sentry/src/test/java/io/sentry/SentryClientTest.kt index 8fab30790f..d5c7a54845 100644 --- a/sentry/src/test/java/io/sentry/SentryClientTest.kt +++ b/sentry/src/test/java/io/sentry/SentryClientTest.kt @@ -138,6 +138,25 @@ class SentryClientTest { assertTrue(sut.isEnabled) } + @Test + fun `when client is closed with isRestarting false, transport waits`() { + val sut = fixture.getSut() + assertTrue(sut.isEnabled) + sut.close(false) + assertNotEquals(0, fixture.sentryOptions.shutdownTimeoutMillis) + verify(fixture.transport).flush(eq(fixture.sentryOptions.shutdownTimeoutMillis)) + verify(fixture.transport).close(eq(false)) + } + + @Test + fun `when client is closed with isRestarting true, transport does not wait`() { + val sut = fixture.getSut() + assertTrue(sut.isEnabled) + sut.close(true) + verify(fixture.transport).flush(eq(0)) + verify(fixture.transport).close(eq(true)) + } + @Test fun `when client is closed, client gets disabled`() { val sut = fixture.getSut() diff --git a/sentry/src/test/java/io/sentry/SentryTest.kt b/sentry/src/test/java/io/sentry/SentryTest.kt index aeed29ce1c..a043fdec62 100644 --- a/sentry/src/test/java/io/sentry/SentryTest.kt +++ b/sentry/src/test/java/io/sentry/SentryTest.kt @@ -62,6 +62,30 @@ class SentryTest { SentryCrashLastRunState.getInstance().reset() } + @Test + fun `init multiple times calls hub close with isRestarting true`() { + val hub = mock() + Sentry.init { + it.dsn = dsn + } + Sentry.setCurrentHub(hub) + Sentry.init { + it.dsn = dsn + } + verify(hub).close(eq(true)) + } + + @Test + fun `close calls hub close with isRestarting false`() { + val hub = mock() + Sentry.init { + it.dsn = dsn + } + Sentry.setCurrentHub(hub) + Sentry.close() + verify(hub).close(eq(false)) + } + @Test fun `outboxPath should be created at initialization`() { var sentryOptions: SentryOptions? = null diff --git a/sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt b/sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt index 182763c161..abaa965175 100644 --- a/sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt +++ b/sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt @@ -16,6 +16,7 @@ import io.sentry.hints.DiskFlushNotification import io.sentry.hints.Enqueable import io.sentry.protocol.SentryId import io.sentry.protocol.User +import io.sentry.test.injectForField import io.sentry.util.HintUtils import org.mockito.kotlin.any import org.mockito.kotlin.anyOrNull @@ -28,6 +29,7 @@ import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import java.io.IOException import java.util.Date +import java.util.concurrent.RejectedExecutionHandler import java.util.concurrent.TimeUnit import kotlin.test.Test import kotlin.test.assertEquals @@ -336,6 +338,39 @@ class AsyncHttpTransportTest { verify(fixture.executor).awaitTermination(eq(123), eq(TimeUnit.MILLISECONDS)) } + @Test + fun `close with isRestarting false uses flushTimeoutMillis option to schedule termination`() { + fixture.sentryOptions.flushTimeoutMillis = 123 + val sut = fixture.getSUT() + sut.close(false) + + verify(fixture.executor).awaitTermination(eq(123), eq(TimeUnit.MILLISECONDS)) + } + + @Test + fun `close with isRestarting true does not await termination`() { + fixture.sentryOptions.flushTimeoutMillis = 123 + val sut = fixture.getSUT() + sut.close(true) + + verify(fixture.executor).awaitTermination(eq(0), eq(TimeUnit.MILLISECONDS)) + } + + @Test + fun `close shuts down the executor and runs executing runnable through rejectedExecutionHandler`() { + val rejectedExecutionHandler = mock() + val sut = fixture.getSUT() + val runnable = mock() + + // Emulate a runnable currently being executed + sut.injectForField("currentRunnable", runnable) + whenever(fixture.executor.rejectedExecutionHandler).thenReturn(rejectedExecutionHandler) + sut.close(true) + + verify(fixture.executor).shutdownNow() + verify(rejectedExecutionHandler).rejectedExecution(eq(runnable), eq(fixture.executor)) + } + @Test fun `when DiskFlushNotification is not flushable, does not flush`() { // given