diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c55dcc5d7..11775c8929 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - Removed SentryExecutorService limit for delayed scheduled tasks ([#4846](https://github.com/getsentry/sentry-java/pull/4846)) - Fix visual artifacts for the Canvas strategy on some devices ([#4861](https://github.com/getsentry/sentry-java/pull/4861)) +- [ANR] Update Connection Status cache in the background ([#4832](https://github.com/getsentry/sentry-java/pull/4832)) ### Improvements diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProvider.java b/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProvider.java index 3f05beeceb..9c8ed2feb3 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProvider.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProvider.java @@ -54,6 +54,7 @@ public final class AndroidConnectionStatusProvider private static final @NotNull AutoClosableReentrantLock childCallbacksLock = new AutoClosableReentrantLock(); private static final @NotNull List childCallbacks = new ArrayList<>(); + private static final AtomicBoolean isUpdatingCache = new AtomicBoolean(false); private static final int[] transports = { NetworkCapabilities.TRANSPORT_WIFI, @@ -146,6 +147,12 @@ private boolean isNetworkEffectivelyConnected( : ConnectionStatus.DISCONNECTED; } + // If cache is being updated, and there's no cached data, return UNKNOWN status until cache is + // updated + if (isUpdatingCache.get()) { + return ConnectionStatus.UNKNOWN; + } + // Fallback to legacy method when NetworkCapabilities not available final ConnectivityManager connectivityManager = getConnectivityManager(context, options.getLogger()); @@ -163,6 +170,12 @@ private boolean isNetworkEffectivelyConnected( return getConnectionType(capabilities); } + // If cache is being updated, and there's no cached data, return UNKNOWN status until cache is + // updated + if (isUpdatingCache.get()) { + return null; + } + // Fallback to legacy method when NetworkCapabilities not available return getConnectionType(context, options.getLogger(), buildInfoProvider); } @@ -268,10 +281,19 @@ private void updateCacheAndNotifyObservers( // Only notify observers if something meaningful changed if (shouldUpdate) { - updateCache(networkCapabilities); - - final @NotNull ConnectionStatus status = getConnectionStatusFromCache(); try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { + cachedNetworkCapabilities = networkCapabilities; + lastCacheUpdateTime = timeProvider.getCurrentTimeMillis(); + final @NotNull ConnectionStatus status = getConnectionStatusFromCache(); + options + .getLogger() + .log( + SentryLevel.DEBUG, + "Cache updated - Status: " + + status + + ", Type: " + + getConnectionTypeFromCache()); + for (final @NotNull IConnectionStatusObserver observer : connectionStatusObservers) { observer.onConnectionStatusChanged(status); @@ -349,62 +371,73 @@ private boolean hasSignificantTransportChanges( } @SuppressLint({"NewApi", "MissingPermission"}) - private void updateCache(@Nullable NetworkCapabilities networkCapabilities) { - try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { - try { - if (networkCapabilities != null) { - cachedNetworkCapabilities = networkCapabilities; - } else { - if (!Permissions.hasPermission(context, Manifest.permission.ACCESS_NETWORK_STATE)) { - options - .getLogger() - .log( - SentryLevel.INFO, - "No permission (ACCESS_NETWORK_STATE) to check network status."); - cachedNetworkCapabilities = null; - lastCacheUpdateTime = timeProvider.getCurrentTimeMillis(); - return; - } + private void updateCache() { + try { + if (!Permissions.hasPermission(context, Manifest.permission.ACCESS_NETWORK_STATE)) { + options + .getLogger() + .log(SentryLevel.INFO, "No permission (ACCESS_NETWORK_STATE) to check network status."); + try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { + cachedNetworkCapabilities = null; + lastCacheUpdateTime = timeProvider.getCurrentTimeMillis(); + } + return; + } - if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.M) { - cachedNetworkCapabilities = null; - lastCacheUpdateTime = timeProvider.getCurrentTimeMillis(); - return; - } + if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.M) { + try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { + cachedNetworkCapabilities = null; + lastCacheUpdateTime = timeProvider.getCurrentTimeMillis(); + } + return; + } - // Fallback: query current active network - final ConnectivityManager connectivityManager = - getConnectivityManager(context, options.getLogger()); - if (connectivityManager != null) { - final Network activeNetwork = connectivityManager.getActiveNetwork(); - - cachedNetworkCapabilities = - activeNetwork != null - ? connectivityManager.getNetworkCapabilities(activeNetwork) - : null; - } else { - cachedNetworkCapabilities = - null; // Clear cached capabilities if connectivity manager is null - } + // Avoid concurrent updates + if (!isUpdatingCache.getAndSet(true)) { + // Fallback: query current active network in the background + if (options.getThreadChecker().isMainThread()) { + submitSafe(() -> updateCacheFromConnectivityManager(), () -> isUpdatingCache.set(false)); + } else { + updateCacheFromConnectivityManager(); } - lastCacheUpdateTime = timeProvider.getCurrentTimeMillis(); + } - options - .getLogger() - .log( - SentryLevel.DEBUG, - "Cache updated - Status: " - + getConnectionStatusFromCache() - + ", Type: " - + getConnectionTypeFromCache()); - } catch (Throwable t) { - options.getLogger().log(SentryLevel.WARNING, "Failed to update connection status cache", t); + } catch (Throwable t) { + options.getLogger().log(SentryLevel.WARNING, "Failed to update connection status cache", t); + try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { cachedNetworkCapabilities = null; lastCacheUpdateTime = timeProvider.getCurrentTimeMillis(); } } } + @RequiresApi(api = Build.VERSION_CODES.M) + private void updateCacheFromConnectivityManager() { + final ConnectivityManager connectivityManager = + getConnectivityManager(context, options.getLogger()); + if (connectivityManager != null) { + final @Nullable NetworkCapabilities capabilities = + getNetworkCapabilities(connectivityManager); + + try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { + cachedNetworkCapabilities = capabilities; + lastCacheUpdateTime = timeProvider.getCurrentTimeMillis(); + + if (capabilities != null) { + options + .getLogger() + .log( + SentryLevel.DEBUG, + "Cache updated - Status: " + + getConnectionStatusFromCache() + + ", Type: " + + getConnectionTypeFromCache()); + } + } + } + isUpdatingCache.set(false); + } + private boolean isCacheValid() { return (timeProvider.getCurrentTimeMillis() - lastCacheUpdateTime) < CACHE_TTL_MS; } @@ -412,7 +445,7 @@ private boolean isCacheValid() { @Override public @NotNull ConnectionStatus getConnectionStatus() { if (!isCacheValid()) { - updateCache(null); + updateCache(); } return getConnectionStatusFromCache(); } @@ -420,7 +453,7 @@ private boolean isCacheValid() { @Override public @Nullable String getConnectionType() { if (!isCacheValid()) { - updateCache(null); + updateCache(); } return getConnectionTypeFromCache(); } @@ -490,7 +523,7 @@ public void onForeground() { () -> { // proactively update cache and notify observers on foreground to ensure connectivity // state is not stale - updateCache(null); + updateCache(); final @NotNull ConnectionStatus status = getConnectionStatusFromCache(); if (status == ConnectionStatus.DISCONNECTED) { @@ -575,6 +608,14 @@ public NetworkCapabilities getCachedNetworkCapabilities() { } } + @RequiresApi(Build.VERSION_CODES.M) + @SuppressLint("MissingPermission") + private static @Nullable NetworkCapabilities getNetworkCapabilities( + final @NotNull ConnectivityManager connectivityManager) { + final Network activeNetwork = connectivityManager.getActiveNetwork(); + return activeNetwork != null ? connectivityManager.getNetworkCapabilities(activeNetwork) : null; + } + /** * Check the connection type of the active network * @@ -603,14 +644,7 @@ public NetworkCapabilities getCachedNetworkCapabilities() { boolean cellular = false; if (buildInfoProvider.getSdkInfoVersion() >= Build.VERSION_CODES.M) { - - final Network activeNetwork = connectivityManager.getActiveNetwork(); - if (activeNetwork == null) { - logger.log(SentryLevel.INFO, "Network is null and cannot check network status"); - return null; - } - final NetworkCapabilities networkCapabilities = - connectivityManager.getNetworkCapabilities(activeNetwork); + final NetworkCapabilities networkCapabilities = getNetworkCapabilities(connectivityManager); if (networkCapabilities == null) { logger.log(SentryLevel.INFO, "NetworkCapabilities is null and cannot check network type"); return null; @@ -820,12 +854,19 @@ public static List getChildCallbacks() { } private void submitSafe(@NotNull Runnable r) { + submitSafe(r, null); + } + + private void submitSafe(final @NotNull Runnable r, final @Nullable Runnable onError) { try { options.getExecutorService().submit(r); } catch (Throwable e) { options .getLogger() .log(SentryLevel.ERROR, "AndroidConnectionStatusProvider submit failed", e); + if (onError != null) { + onError.run(); + } } } } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderTest.kt index 4dd8062464..a1a1e6550a 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderTest.kt @@ -25,8 +25,10 @@ import io.sentry.android.core.AppState import io.sentry.android.core.BuildInfoProvider import io.sentry.android.core.ContextUtils import io.sentry.android.core.SystemEventsBreadcrumbsIntegration +import io.sentry.test.DeferredExecutorService import io.sentry.test.ImmediateExecutorService import io.sentry.transport.ICurrentDateProvider +import io.sentry.util.thread.IThreadChecker import kotlin.test.AfterTest import kotlin.test.BeforeTest import kotlin.test.Test @@ -38,6 +40,7 @@ import kotlin.test.assertTrue import org.junit.runner.RunWith import org.mockito.MockedStatic import org.mockito.Mockito.mockStatic +import org.mockito.Mockito.never import org.mockito.kotlin.any import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.argThat @@ -103,6 +106,7 @@ class AndroidConnectionStatusProviderTest { options = SentryOptions() options.setLogger(logger) options.executorService = ImmediateExecutorService() + options.threadChecker = AndroidThreadChecker.getInstance() // Reset current time for each test to ensure cache isolation currentTime = 1000L @@ -125,6 +129,7 @@ class AndroidConnectionStatusProviderTest { @AfterTest fun `tear down`() { + options.executorService = ImmediateExecutorService() // clear the cache and ensure proper cleanup connectionStatusProvider.close() contextUtilsStaticMock.close() @@ -205,6 +210,41 @@ class AndroidConnectionStatusProviderTest { providerWithNullConnectivity.close() } + @Test + fun `When cache is updating, return UNKNOWN for connectionStatus on main thread`() { + whenever(networkInfo.isConnected).thenReturn(true) + // When we are on the main thread + val mockThreadChecker = mock() + options.threadChecker = mockThreadChecker + whenever(mockThreadChecker.isMainThread()).thenReturn(true) + + // The update is done on the background + val executorService = DeferredExecutorService() + options.executorService = executorService + + // Advance time beyond TTL (2 minutes) + currentTime += 2 * 60 * 1000L + + // Connection status is unknown while we update the cache + assertEquals( + IConnectionStatusProvider.ConnectionStatus.UNKNOWN, + connectionStatusProvider.connectionStatus, + ) + + verify(connectivityManager, never()).activeNetworkInfo + verify(connectivityManager, never()).activeNetwork + + // When background cache update is done + executorService.runAll() + + // Connection status is updated + verify(connectivityManager).activeNetwork + assertEquals( + IConnectionStatusProvider.ConnectionStatus.CONNECTED, + connectionStatusProvider.connectionStatus, + ) + } + @Test fun `When there's no permission, return null for getConnectionType`() { whenever(contextMock.checkPermission(any(), any(), any())).thenReturn(PERMISSION_DENIED) @@ -219,6 +259,35 @@ class AndroidConnectionStatusProviderTest { assertNull(connectionStatusProvider.connectionType) } + @Test + fun `When cache is updating, return null for getConnectionType on main thread`() { + whenever(networkInfo.isConnected).thenReturn(true) + // When we are on the main thread + val mockThreadChecker = mock() + options.threadChecker = mockThreadChecker + whenever(mockThreadChecker.isMainThread()).thenReturn(true) + + // The update is done on the background + val executorService = DeferredExecutorService() + options.executorService = executorService + + // Advance time beyond TTL (2 minutes) + currentTime += 2 * 60 * 1000L + + // Connection type is null while we update the cache + assertNull(connectionStatusProvider.connectionType) + + verify(connectivityManager, never()).activeNetworkInfo + verify(connectivityManager, never()).activeNetwork + + // When background cache update is done + executorService.runAll() + + // Connection type is updated + verify(connectivityManager).activeNetwork + assertNotNull(connectionStatusProvider.connectionType) + } + @Test fun `When network capabilities are not available, return null for getConnectionType`() { whenever(connectivityManager.getNetworkCapabilities(any())).thenReturn(null)