Merge be25954 into 5e04ee8

getsentry · Feb 8, 2024 · 0e03bb0 · 0e03bb0
2 parents 5e04ee8 + be25954
commit 0e03bb0
Show file tree

Hide file tree

Showing 28 changed files with 353 additions and 14 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,10 @@
 
 - Add new threshold parameters to monitor config ([#3181](https://github.com/getsentry/sentry-java/pull/3181))
 
+### Fixes
+
+- Don't wait on main thread when SDK restarts ([#3200](https://github.com/getsentry/sentry-java/pull/3200))
+
 ## 7.3.0
 
 ### Features

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
@@ -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
@@ -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/...ests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt b/...ests/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/...tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/SdkInitTests.kt b/...tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/SdkInitTests.kt
@@ -9,9 +9,12 @@ import io.sentry.android.core.AndroidLogger
 import io.sentry.android.core.SentryAndroidOptions
 import io.sentry.assertEnvelopeTransaction
 import io.sentry.protocol.SentryTransaction
+import okhttp3.mockwebserver.MockResponse
+import okhttp3.mockwebserver.SocketPolicy
 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 +77,101 @@ class SdkInitTests : BaseUiTest() {
             assertNoOtherEnvelopes()
         }
     }
+
+    @Test
+    fun doubleInitDoesNotWait() {
+        relayIdlingResource.increment()
+        relayIdlingResource.increment()
+        // Let's make the first request timeout
+        relay.addResponse { MockResponse().setSocketPolicy(SocketPolicy.NO_RESPONSE) }
+
+        initSentry(true) { options: SentryAndroidOptions ->
+            options.tracesSampleRate = 1.0
+        }
+
+        Sentry.startTransaction("beforeRestart", "emptyTransaction").finish()
+
+        val beforeRestart = System.currentTimeMillis()
+        // We restart the SDK. This shouldn't block the main thread, but new options (e.g. profiling) should work
+        initSentry(false) { options: SentryAndroidOptions ->
+            // Registering again the idling resource blocks for some time. But we already registered it before.
+            // So we just need to update the mock relay flag, as we are overwriting it in this "initSentry(false)".
+            relay.waitForRequests = true
+            options.tracesSampleRate = 1.0
+            options.profilesSampleRate = 1.0
+        }
+        val afterRestart = System.currentTimeMillis()
+        val restartMs = afterRestart - beforeRestart
+
+        Sentry.startTransaction("afterRestart", "emptyTransaction").finish()
+        assertTrue(restartMs < 250, "Expected less than 250 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()
+        relayIdlingResource.increment()
+        // Let's make the first request timeout
+        relay.addResponse { MockResponse().setSocketPolicy(SocketPolicy.NO_RESPONSE) }
+
+        initSentry(true) { options: SentryAndroidOptions ->
+            options.tracesSampleRate = 1.0
+        }
+
+        Sentry.startTransaction("beforeRestart", "emptyTransaction").finish()
+
+        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(false) { options: SentryAndroidOptions ->
+            // Registering again the idling resource blocks for some time. But we already registered it before.
+            // So we just need to update the mock relay flag, as we are overwriting it in this "initSentry(false)".
+            relay.waitForRequests = true
+            options.tracesSampleRate = 1.0
+            options.profilesSampleRate = 1.0
+        }
+        val afterRestart = System.currentTimeMillis()
+        val restartMs = afterRestart - beforeRestart
+
+        Sentry.startTransaction("afterRestart", "emptyTransaction").finish()
+        assertTrue(restartMs > 2000, "Expected more than 2000 ms for SDK close and 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()
+        }
+    }
 }
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
@@ -1,6 +1,7 @@
 public final class io/sentry/transport/apache/ApacheHttpClientTransport : io/sentry/transport/ITransport {
 	public fun <init> (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/...che-http-client-5/src/main/java/io/sentry/transport/apache/ApacheHttpClientTransport.java b/...che-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/...http-client-5/src/test/kotlin/io/sentry/transport/apache/ApacheHttpClientTransportTest.kt b/...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
@@ -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 <init> (Lio/sentry/SentryOptions;Lio/sentry/transport/RateLimiter;Lio/sentry/transport/ITransportGate;Lio/sentry/RequestDetails;)V
 	public fun <init> (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 <init> (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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -28,6 +28,9 @@ public boolean isEnabled() {
     return SentryId.EMPTY_ID;
   }
 
+  @Override
+  public void close(final boolean isRestarting) {}
+
   @Override
   public void close() {}