Skip to content

Commit fde11a2

Browse files
romtsnromtsn
committed
perf(integrations): Do not register for SystemEvents and NetworkCallbacks when launched with background importance
1 parent 42ff8e9 commit fde11a2

10 files changed

Lines changed: 430 additions & 60 deletions

File tree

‎sentry-android-core/src/main/java/io/sentry/android/core/AppState.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package io.sentry.android.core;
22

3+
import android.util.Log;
34
import androidx.annotation.NonNull;
45
import androidx.lifecycle.DefaultLifecycleObserver;
56
import androidx.lifecycle.LifecycleOwner;
@@ -48,15 +49,15 @@ void setInBackground(final boolean inBackground) {
4849
this.inBackground = inBackground;
4950
}
5051

51-
void addAppStateListener(final @NotNull AppStateListener listener) {
52+
public void addAppStateListener(final @NotNull AppStateListener listener) {
5253
try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) {
5354
ensureLifecycleObserver(NoOpLogger.getInstance());
5455

5556
lifecycleObserver.listeners.add(listener);
5657
}
5758
}
5859

59-
void removeAppStateListener(final @NotNull AppStateListener listener) {
60+
public void removeAppStateListener(final @NotNull AppStateListener listener) {
6061
try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) {
6162
if (lifecycleObserver != null) {
6263
lifecycleObserver.listeners.remove(listener);
@@ -172,10 +173,10 @@ public boolean add(AppStateListener appStateListener) {
172173

173174
@Override
174175
public void onStart(@NonNull LifecycleOwner owner) {
176+
setInBackground(false);
175177
for (AppStateListener listener : listeners) {
176178
listener.onForeground();
177179
}
178-
setInBackground(false);
179180
}
180181

181182
@Override

‎sentry-android-core/src/main/java/io/sentry/android/core/SystemEventsBreadcrumbsIntegration.java‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import android.content.Intent;
2626
import android.content.IntentFilter;
2727
import android.os.Bundle;
28+
import android.util.Log;
2829
import io.sentry.Breadcrumb;
2930
import io.sentry.Hint;
3031
import io.sentry.IScopes;
@@ -43,6 +44,7 @@
4344
import java.util.HashMap;
4445
import java.util.List;
4546
import java.util.Map;
47+
import java.util.concurrent.atomic.AtomicBoolean;
4648
import org.jetbrains.annotations.NotNull;
4749
import org.jetbrains.annotations.Nullable;
4850
import org.jetbrains.annotations.TestOnly;
@@ -62,6 +64,7 @@ public final class SystemEventsBreadcrumbsIntegration
6264
private volatile boolean isClosed = false;
6365
private volatile boolean isStopped = false;
6466
private volatile IntentFilter filter = null;
67+
private final @NotNull AtomicBoolean isReceiverRegistered = new AtomicBoolean(false);
6568
private final @NotNull AutoClosableReentrantLock receiverLock = new AutoClosableReentrantLock();
6669

6770
public SystemEventsBreadcrumbsIntegration(final @NotNull Context context) {
@@ -99,14 +102,16 @@ public void register(final @NotNull IScopes scopes, final @NotNull SentryOptions
99102

100103
if (this.options.isEnableSystemEventBreadcrumbs()) {
101104
AppState.getInstance().addAppStateListener(this);
102-
registerReceiver(this.scopes, this.options, /* reportAsNewIntegration= */ true);
105+
106+
if (ContextUtils.isForegroundImportance()) {
107+
registerReceiver(this.scopes, this.options);
108+
}
103109
}
104110
}
105111

106112
private void registerReceiver(
107113
final @NotNull IScopes scopes,
108-
final @NotNull SentryAndroidOptions options,
109-
final boolean reportAsNewIntegration) {
114+
final @NotNull SentryAndroidOptions options) {
110115

111116
if (!options.isEnableSystemEventBreadcrumbs()) {
112117
return;
@@ -137,7 +142,7 @@ private void registerReceiver(
137142
// registerReceiver can throw SecurityException but it's not documented in the
138143
// official docs
139144
ContextUtils.registerReceiver(context, options, receiver, filter);
140-
if (reportAsNewIntegration) {
145+
if (!isReceiverRegistered.getAndSet(true)) {
141146
options
142147
.getLogger()
143148
.log(SentryLevel.DEBUG, "SystemEventsBreadcrumbsIntegration installed.");
@@ -237,7 +242,7 @@ public void onForeground() {
237242

238243
isStopped = false;
239244

240-
registerReceiver(scopes, options, /* reportAsNewIntegration= */ false);
245+
registerReceiver(scopes, options);
241246
}
242247

243248
@Override

‎sentry-android-core/src/main/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProvider.java‎

Lines changed: 118 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,20 @@
1313
import io.sentry.IConnectionStatusProvider;
1414
import io.sentry.ILogger;
1515
import io.sentry.ISentryLifecycleToken;
16+
import io.sentry.Sentry;
1617
import io.sentry.SentryLevel;
1718
import io.sentry.SentryOptions;
19+
import io.sentry.android.core.AppState;
1820
import io.sentry.android.core.BuildInfoProvider;
1921
import io.sentry.android.core.ContextUtils;
2022
import io.sentry.transport.ICurrentDateProvider;
2123
import io.sentry.util.AutoClosableReentrantLock;
2224
import java.util.ArrayList;
2325
import java.util.List;
26+
import java.util.concurrent.ExecutorService;
27+
import java.util.concurrent.Executors;
28+
import java.util.concurrent.ThreadFactory;
29+
import java.util.concurrent.atomic.AtomicBoolean;
2430
import org.jetbrains.annotations.ApiStatus;
2531
import org.jetbrains.annotations.NotNull;
2632
import org.jetbrains.annotations.Nullable;
@@ -32,7 +38,8 @@
3238
* details
3339
*/
3440
@ApiStatus.Internal
35-
public final class AndroidConnectionStatusProvider implements IConnectionStatusProvider {
41+
public final class AndroidConnectionStatusProvider implements IConnectionStatusProvider,
42+
AppState.AppStateListener {
3643

3744
private final @NotNull Context context;
3845
private final @NotNull SentryOptions options;
@@ -58,12 +65,12 @@ public final class AndroidConnectionStatusProvider implements IConnectionStatusP
5865

5966
private static final int[] capabilities = new int[2];
6067

61-
private final @NotNull Thread initThread;
6268
private volatile @Nullable NetworkCapabilities cachedNetworkCapabilities;
6369
private volatile @Nullable Network currentNetwork;
6470
private volatile long lastCacheUpdateTime = 0;
6571
private static final long CACHE_TTL_MS = 2 * 60 * 1000L; // 2 minutes
66-
72+
private final @NotNull AtomicBoolean isConnected = new AtomicBoolean(false);
73+
6774
@SuppressLint("InlinedApi")
6875
public AndroidConnectionStatusProvider(
6976
@NotNull Context context,
@@ -83,8 +90,9 @@ public AndroidConnectionStatusProvider(
8390

8491
// Register network callback immediately for caching
8592
//noinspection Convert2MethodRef
86-
initThread = new Thread(() -> ensureNetworkCallbackRegistered());
87-
initThread.start();
93+
submitSafe(() -> ensureNetworkCallbackRegistered());
94+
95+
AppState.getInstance().addAppStateListener(this);
8896
}
8997

9098
/**
@@ -148,6 +156,10 @@ private boolean isNetworkEffectivelyConnected(
148156
}
149157

150158
private void ensureNetworkCallbackRegistered() {
159+
if (!ContextUtils.isForegroundImportance()) {
160+
return;
161+
}
162+
151163
if (networkCallback != null) {
152164
return; // Already registered
153165
}
@@ -163,9 +175,13 @@ private void ensureNetworkCallbackRegistered() {
163175
public void onAvailable(final @NotNull Network network) {
164176
currentNetwork = network;
165177

166-
try (final @NotNull ISentryLifecycleToken ignored = childCallbacksLock.acquire()) {
167-
for (final @NotNull NetworkCallback cb : childCallbacks) {
168-
cb.onAvailable(network);
178+
// have to only dispatch this on first registration + when the connection got re-established
179+
// otherwise it would've been dispatched on every foreground
180+
if (!isConnected.getAndSet(true)) {
181+
try (final @NotNull ISentryLifecycleToken ignored = childCallbacksLock.acquire()) {
182+
for (final @NotNull NetworkCallback cb : childCallbacks) {
183+
cb.onAvailable(network);
184+
}
169185
}
170186
}
171187
}
@@ -197,6 +213,7 @@ public void onLost(final @NotNull Network network) {
197213
}
198214

199215
private void clearCacheAndNotifyObservers() {
216+
isConnected.set(false);
200217
// Clear cached capabilities and network reference atomically
201218
try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) {
202219
cachedNetworkCapabilities = null;
@@ -413,42 +430,84 @@ public void removeConnectionStatusObserver(final @NotNull IConnectionStatusObser
413430
}
414431
}
415432

433+
private void unregisterNetworkCallback(final boolean clearObservers) {
434+
try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) {
435+
if (clearObservers) {
436+
connectionStatusObservers.clear();
437+
}
438+
439+
final @Nullable NetworkCallback callbackRef = networkCallback;
440+
networkCallback = null;
441+
442+
if (callbackRef != null) {
443+
unregisterNetworkCallback(context, options.getLogger(), callbackRef);
444+
}
445+
// Clear cached state
446+
cachedNetworkCapabilities = null;
447+
currentNetwork = null;
448+
lastCacheUpdateTime = 0;
449+
}
450+
options.getLogger().log(SentryLevel.DEBUG, "Network callback unregistered");
451+
}
452+
416453
/** Clean up resources - should be called when the provider is no longer needed */
417454
@Override
418455
public void close() {
419-
try {
420-
options
421-
.getExecutorService()
422-
.submit(
423-
() -> {
424-
final NetworkCallback callbackRef;
425-
try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) {
426-
connectionStatusObservers.clear();
456+
submitSafe(
457+
() -> {
458+
unregisterNetworkCallback(/* clearObservers = */true);
459+
try (final @NotNull ISentryLifecycleToken ignored = childCallbacksLock.acquire()) {
460+
childCallbacks.clear();
461+
}
462+
try (final @NotNull ISentryLifecycleToken ignored =
463+
connectivityManagerLock.acquire()) {
464+
connectivityManager = null;
465+
}
466+
});
467+
}
427468

428-
callbackRef = networkCallback;
429-
networkCallback = null;
469+
@Override
470+
public void onForeground() {
471+
if (networkCallback != null) {
472+
return;
473+
}
430474

431-
if (callbackRef != null) {
432-
unregisterNetworkCallback(context, options.getLogger(), callbackRef);
433-
}
434-
// Clear cached state
435-
cachedNetworkCapabilities = null;
436-
currentNetwork = null;
437-
lastCacheUpdateTime = 0;
438-
}
439-
try (final @NotNull ISentryLifecycleToken ignored = childCallbacksLock.acquire()) {
440-
childCallbacks.clear();
441-
}
442-
try (final @NotNull ISentryLifecycleToken ignored =
443-
connectivityManagerLock.acquire()) {
444-
connectivityManager = null;
445-
}
446-
});
447-
} catch (Throwable t) {
448-
options
449-
.getLogger()
450-
.log(SentryLevel.ERROR, "Error submitting AndroidConnectionStatusProvider task", t);
475+
submitSafe(() -> {
476+
// proactively update cache and notify observers on foreground to ensure connectivity state is not stale
477+
updateCache(null);
478+
479+
final @NotNull ConnectionStatus status = getConnectionStatusFromCache();
480+
if (status == ConnectionStatus.DISCONNECTED) {
481+
// onLost is not called retroactively when we registerNetworkCallback (as opposed to onAvailable), so we have to do it manually for the DISCONNECTED case
482+
isConnected.set(false);
483+
try (final @NotNull ISentryLifecycleToken ignored = childCallbacksLock.acquire()) {
484+
for (final @NotNull NetworkCallback cb : childCallbacks) {
485+
//noinspection DataFlowIssue
486+
cb.onLost(null);
487+
}
488+
}
489+
}
490+
try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) {
491+
for (final @NotNull IConnectionStatusObserver observer :
492+
connectionStatusObservers) {
493+
observer.onConnectionStatusChanged(status);
494+
}
495+
}
496+
497+
ensureNetworkCallbackRegistered();
498+
});
499+
}
500+
501+
@Override
502+
public void onBackground() {
503+
if (networkCallback == null) {
504+
return;
451505
}
506+
507+
submitSafe(() -> {
508+
//noinspection Convert2MethodRef
509+
unregisterNetworkCallback(/* clearObservers = */false);
510+
});
452511
}
453512

454513
/**
@@ -595,7 +654,6 @@ public NetworkCapabilities getCachedNetworkCapabilities() {
595654
if (cellular) {
596655
return "cellular";
597656
}
598-
599657
} catch (Throwable exception) {
600658
logger.log(SentryLevel.ERROR, "Failed to retrieve network info", exception);
601659
}
@@ -730,15 +788,30 @@ public NetworkCallback getNetworkCallback() {
730788
return networkCallback;
731789
}
732790

733-
@TestOnly
734-
@NotNull
735-
public Thread getInitThread() {
736-
return initThread;
737-
}
738-
739791
@TestOnly
740792
@NotNull
741793
public static List<NetworkCallback> getChildCallbacks() {
742794
return childCallbacks;
743795
}
796+
797+
private void submitSafe(@NotNull Runnable r) {
798+
try {
799+
options.getExecutorService().submit(r);
800+
} catch (Throwable e) {
801+
options.getLogger()
802+
.log(SentryLevel.ERROR, "AndroidConnectionStatusProvider submit failed", e);
803+
}
804+
}
805+
806+
private static class ConnectionStatusProviderThreadyFactory implements ThreadFactory {
807+
808+
private int cnt = 0;
809+
810+
@Override
811+
public Thread newThread(Runnable r) {
812+
final Thread ret = new Thread(r, "SentryConnectionStatusProvider-" + cnt++);
813+
ret.setDaemon(true);
814+
return ret;
815+
}
816+
}
744817
}

‎sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderTest.kt‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import kotlin.test.assertNull
3131
import kotlin.test.assertTrue
3232
import org.mockito.kotlin.any
3333
import org.mockito.kotlin.argumentCaptor
34+
import org.mockito.kotlin.doAnswer
3435
import org.mockito.kotlin.eq
3536
import org.mockito.kotlin.mock
3637
import org.mockito.kotlin.mockingDetails
@@ -39,6 +40,7 @@ import org.mockito.kotlin.verify
3940
import org.mockito.kotlin.verifyNoInteractions
4041
import org.mockito.kotlin.verifyNoMoreInteractions
4142
import org.mockito.kotlin.whenever
43+
import java.util.concurrent.ExecutorService
4244

4345
class AndroidConnectionStatusProviderTest {
4446
private lateinit var connectionStatusProvider: AndroidConnectionStatusProvider
@@ -93,9 +95,6 @@ class AndroidConnectionStatusProviderTest {
9395

9496
connectionStatusProvider =
9597
AndroidConnectionStatusProvider(contextMock, options, buildInfo, timeProvider)
96-
97-
// Wait for async callback registration to complete
98-
connectionStatusProvider.initThread.join()
9998
}
10099

101100
@AfterTest
@@ -164,7 +163,6 @@ class AndroidConnectionStatusProviderTest {
164163
// Create a new provider with the null connectivity manager
165164
val providerWithNullConnectivity =
166165
AndroidConnectionStatusProvider(nullConnectivityContext, options, buildInfo, timeProvider)
167-
providerWithNullConnectivity.initThread.join() // Wait for async init to complete
168166

169167
assertEquals(
170168
IConnectionStatusProvider.ConnectionStatus.UNKNOWN,

0 commit comments

Comments
 (0)