Skip to content

Commit f33a11d

Browse files
stefanosianostefanosiano
authored
Don't wait on main thread when SDK restarts (getsentry#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
1 parent ceb541b commit f33a11d

30 files changed

Lines changed: 385 additions & 27 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,9 @@
77
- Add new threshold parameters to monitor config ([#3181](https://github.com/getsentry/sentry-java/pull/3181))
88
- Report process init time as a span for app start performance ([#3159](https://github.com/getsentry/sentry-java/pull/3159))
99

10-
## Fixes
10+
### Fixes
1111

12+
- Don't wait on main thread when SDK restarts ([#3200](https://github.com/getsentry/sentry-java/pull/3200))
1213
- Fix Jetpack Compose widgets are not being correctly identified for user interaction tracing ([#3209](https://github.com/getsentry/sentry-java/pull/3209))
1314

1415
## 7.3.0

‎sentry-android-core/src/test/java/io/sentry/android/core/InternalSentrySdkTest.kt‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,10 @@ class InternalSentrySdkTest {
5454
options.dsn = "https://key@host/proj"
5555
options.setTransportFactory { _, _ ->
5656
object : ITransport {
57+
override fun close(isRestarting: Boolean) {
58+
// no-op
59+
}
60+
5761
override fun close() {
5862
// no-op
5963
}

‎sentry-android-core/src/test/java/io/sentry/android/core/SessionTrackingIntegrationTest.kt‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,10 @@ class SessionTrackingIntegrationTest {
133133
TODO("Not yet implemented")
134134
}
135135

136+
override fun close(isRestarting: Boolean) {
137+
TODO("Not yet implemented")
138+
}
139+
136140
override fun close() {
137141
TODO("Not yet implemented")
138142
}

‎sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/EnvelopeTests.kt‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
package io.sentry.uitest.android
22

3-
import android.util.Log
43
import androidx.lifecycle.Lifecycle
54
import androidx.test.core.app.launchActivity
65
import androidx.test.espresso.Espresso
@@ -199,7 +198,6 @@ class EnvelopeTests : BaseUiTest() {
199198

200199
relay.assert {
201200
findEnvelope {
202-
Log.e("ITEMS", it.items.joinToString { item -> item.header.type.itemType })
203201
assertEnvelopeTransaction(it.items.toList(), AndroidLogger()).transaction == "timedOutProfile"
204202
}.assert {
205203
val transactionItem: SentryTransaction = it.assertTransaction()

‎sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/SdkInitTests.kt‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import io.sentry.protocol.SentryTransaction
1212
import org.junit.runner.RunWith
1313
import kotlin.test.Test
1414
import kotlin.test.assertEquals
15+
import kotlin.test.assertTrue
1516

1617
@RunWith(AndroidJUnit4::class)
1718
class SdkInitTests : BaseUiTest() {
@@ -74,4 +75,83 @@ class SdkInitTests : BaseUiTest() {
7475
assertNoOtherEnvelopes()
7576
}
7677
}
78+
79+
@Test
80+
fun doubleInitDoesNotWait() {
81+
relayIdlingResource.increment()
82+
// Let's make the first request timeout
83+
relay.addTimeoutResponse()
84+
85+
initSentry(true) { options: SentryAndroidOptions ->
86+
options.tracesSampleRate = 1.0
87+
}
88+
89+
Sentry.startTransaction("beforeRestart", "emptyTransaction").finish()
90+
91+
// 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
92+
waitUntilIdle()
93+
94+
relayIdlingResource.increment()
95+
relayIdlingResource.increment()
96+
97+
val beforeRestart = System.currentTimeMillis()
98+
// We restart the SDK. This shouldn't block the main thread, but new options (e.g. profiling) should work
99+
initSentry(true) { options: SentryAndroidOptions ->
100+
options.tracesSampleRate = 1.0
101+
options.profilesSampleRate = 1.0
102+
}
103+
val afterRestart = System.currentTimeMillis()
104+
val restartMs = afterRestart - beforeRestart
105+
106+
Sentry.startTransaction("afterRestart", "emptyTransaction").finish()
107+
// We assert for less than 1 second just to account for slow devices in saucelabs or headless emulator
108+
assertTrue(restartMs < 1000, "Expected less than 1000 ms for SDK restart. Got $restartMs ms")
109+
110+
relay.assert {
111+
findEnvelope {
112+
assertEnvelopeTransaction(it.items.toList()).transaction == "beforeRestart"
113+
}.assert {
114+
it.assertTransaction()
115+
// No profiling item, as in the first init it was not enabled
116+
it.assertNoOtherItems()
117+
}
118+
findEnvelope {
119+
assertEnvelopeTransaction(it.items.toList()).transaction == "afterRestart"
120+
}.assert {
121+
it.assertTransaction()
122+
// There is a profiling item, as in the second init it was enabled
123+
it.assertProfile()
124+
it.assertNoOtherItems()
125+
}
126+
assertNoOtherEnvelopes()
127+
}
128+
}
129+
130+
@Test
131+
fun initCloseInitWaits() {
132+
relayIdlingResource.increment()
133+
// Let's make the first request timeout
134+
relay.addTimeoutResponse()
135+
136+
initSentry(true) { options: SentryAndroidOptions ->
137+
options.tracesSampleRate = 1.0
138+
options.flushTimeoutMillis = 3000
139+
}
140+
141+
Sentry.startTransaction("beforeRestart", "emptyTransaction").finish()
142+
143+
// 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
144+
waitUntilIdle()
145+
146+
val beforeRestart = System.currentTimeMillis()
147+
Sentry.close()
148+
// We stop the SDK. This should block the main thread. Then we start it again with new options
149+
initSentry(true) { options: SentryAndroidOptions ->
150+
options.tracesSampleRate = 1.0
151+
options.profilesSampleRate = 1.0
152+
}
153+
val afterRestart = System.currentTimeMillis()
154+
val restartMs = afterRestart - beforeRestart
155+
assertTrue(restartMs > 3000, "Expected more than 3000 ms for SDK close and restart. Got $restartMs ms")
156+
}
77157
}

‎sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/mockservers/MockRelay.kt‎

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import okhttp3.mockwebserver.Dispatcher
66
import okhttp3.mockwebserver.MockResponse
77
import okhttp3.mockwebserver.MockWebServer
88
import okhttp3.mockwebserver.RecordedRequest
9+
import okhttp3.mockwebserver.SocketPolicy
910
import kotlin.test.assertNotNull
1011

1112
/** Mocks a relay server. */
@@ -32,24 +33,24 @@ class MockRelay(
3233
init {
3334
relay.dispatcher = object : Dispatcher() {
3435
override fun dispatch(request: RecordedRequest): MockResponse {
35-
// If a request with a body size of 0 is received, we drop it.
36-
// This shouldn't happen in reality, but it rarely happens in tests.
37-
if (request.bodySize == 0L || request.failure != null) {
38-
return MockResponse()
39-
}
4036
// We check if there is any custom response previously set to return to this request,
4137
// otherwise we return a successful MockResponse.
42-
val response = responses.asSequence()
43-
.mapNotNull { it(request) }
44-
.firstOrNull()
45-
?: MockResponse()
38+
val response = responses.removeFirstOrNull()?.let { it(request) } ?: MockResponse()
4639

4740
// We should receive only envelopes on this path.
4841
if (request.path == envelopePath) {
4942
val relayResponse = RelayAsserter.RelayResponse(request, response)
43+
// If we reply with NO_RESPONSE, we can ignore the request, so we can return here
44+
if (relayResponse.envelope == null || response.socketPolicy == SocketPolicy.NO_RESPONSE) {
45+
// If we are waiting for requests to be received, we decrement the associated counter.
46+
if (waitForRequests) {
47+
relayIdlingResource.decrement()
48+
}
49+
return response
50+
}
5051
assertNotNull(relayResponse.envelope)
5152
val envelopeId: String = relayResponse.envelope!!.header.eventId!!.toString()
52-
// If we already received the envelope (e.g. retrying mechanism) we drop it
53+
// If we already received the envelope (e.g. retrying mechanism) we ignore it
5354
if (receivedEnvelopes.contains(envelopeId)) {
5455
return MockResponse()
5556
}
@@ -90,6 +91,13 @@ class MockRelay(
9091
responses.add(0, response)
9192
}
9293

94+
/** Add a custom response to be returned at the next request received. */
95+
fun addTimeoutResponse() {
96+
addResponse {
97+
MockResponse().setSocketPolicy(SocketPolicy.NO_RESPONSE)
98+
}
99+
}
100+
93101
/** Add a custom response to be returned at the next request received, if it satisfies the [filter]. */
94102
fun addResponse(
95103
filter: (RecordedRequest) -> Boolean,

‎sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/mockservers/RelayAsserter.kt‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,13 @@ class RelayAsserter(
4040
filter: (envelope: SentryEnvelope) -> Boolean = { true }
4141
): RelayResponse {
4242
val relayResponseIndex = unassertedEnvelopes.indexOfFirst { it.envelope?.let(filter) ?: false }
43-
if (relayResponseIndex == -1) throw AssertionError("No envelope request found with specified filter")
43+
if (relayResponseIndex == -1) {
44+
throw AssertionError(
45+
"No envelope request found with specified filter.\n" +
46+
"There was a total of ${originalUnassertedEnvelopes.size} envelopes: " +
47+
originalUnassertedEnvelopes.joinToString { describeEnvelope(it.envelope!!) }
48+
)
49+
}
4450
return unassertedEnvelopes.removeAt(relayResponseIndex)
4551
}
4652

‎sentry-apache-http-client-5/api/sentry-apache-http-client-5.api‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
public final class io/sentry/transport/apache/ApacheHttpClientTransport : io/sentry/transport/ITransport {
22
public fun <init> (Lio/sentry/SentryOptions;Lio/sentry/RequestDetails;Lorg/apache/hc/client5/http/impl/async/CloseableHttpAsyncClient;Lio/sentry/transport/RateLimiter;)V
33
public fun close ()V
4+
public fun close (Z)V
45
public fun flush (J)V
56
public fun getRateLimiter ()Lio/sentry/transport/RateLimiter;
67
public fun send (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V

‎sentry-apache-http-client-5/src/main/java/io/sentry/transport/apache/ApacheHttpClientTransport.java‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -196,9 +196,14 @@ public void flush(long timeoutMillis) {
196196

197197
@Override
198198
public void close() throws IOException {
199+
close(false);
200+
}
201+
202+
@Override
203+
public void close(final boolean isRestarting) throws IOException {
199204
options.getLogger().log(DEBUG, "Shutting down");
200205
try {
201-
httpclient.awaitShutdown(TimeValue.ofSeconds(1));
206+
httpclient.awaitShutdown(TimeValue.ofSeconds(isRestarting ? 0 : 1));
202207
} catch (InterruptedException e) {
203208
options.getLogger().log(DEBUG, "Thread interrupted while closing the connection.");
204209
Thread.currentThread().interrupt();

‎sentry-apache-http-client-5/src/test/kotlin/io/sentry/transport/apache/ApacheHttpClientTransportTest.kt‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import java.util.concurrent.Executors
3131
import kotlin.test.AfterTest
3232
import kotlin.test.Test
3333
import kotlin.test.assertEquals
34+
import kotlin.test.assertNotEquals
3435

3536
class ApacheHttpClientTransportTest {
3637

@@ -116,7 +117,21 @@ class ApacheHttpClientTransportTest {
116117
fun `close waits for shutdown`() {
117118
val sut = fixture.getSut()
118119
sut.close()
119-
verify(fixture.client).awaitShutdown(any())
120+
verify(fixture.client).awaitShutdown(check { assertNotEquals(0L, it.duration) })
121+
}
122+
123+
@Test
124+
fun `close with isRestarting false waits for shutdown`() {
125+
val sut = fixture.getSut()
126+
sut.close(false)
127+
verify(fixture.client).awaitShutdown(check { assertNotEquals(0L, it.duration) })
128+
}
129+
130+
@Test
131+
fun `close with isRestarting true does not wait for shutdown`() {
132+
val sut = fixture.getSut()
133+
sut.close(true)
134+
verify(fixture.client).awaitShutdown(check { assertEquals(0L, it.duration) })
120135
}
121136

122137
@Test

0 commit comments

Comments
 (0)