From d2f231b3350b602d0aec48140aec4981cba5431a Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Mon, 9 Nov 2026 11:41:32 +0100 Subject: [PATCH 01/10] fix(android): Don't block callers on connection status registration AndroidConnectionStatusProvider held its lock on the SDK executor while registering network and telephony callbacks, which are slow system service calls. The main thread waited on the same lock when Session Replay added an observer during Sentry.init, and when events were captured with a stale connection cache, causing app-start ANRs. Registration now uses its own executor-only lock. The cache lock is held only for field writes, and cache refreshes query the active network outside it, discarding their result if a network callback wrote the cache meanwhile. Observers and child network callbacks are copy-on-write lists, notified without holding a lock. addConnectionStatusObserver schedules registration instead of running it on the caller, and always returns true. Lifecycle callbacks now check registration state on the executor, so backgrounding during the initial registration unregisters the callbacks, and foregrounding before a queued unregistration runs registers them again. Fixes #6509 --- CHANGELOG.md | 1 + .../util/AndroidConnectionStatusProvider.java | 332 +++++---- ...ConnectionStatusProviderConcurrencyTest.kt | 679 ++++++++++++++++++ .../AndroidConnectionStatusProviderTest.kt | 18 + 4 files changed, 896 insertions(+), 134 deletions(-) create mode 100644 sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderConcurrencyTest.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a08fb1ae6c..cde07fbb999 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ ### Fixes +- Prevent the main thread from potentially hanging during app start if Android is slow to register the SDK's network or telephony listeners. `Sentry.init`, Session Replay, and capturing events no longer wait for that registration ([#XXXX](https://github.com/getsentry/sentry-java/issues/XXXX)) - Add opt-in support for matching GraphQL ignored error types against classification specifications via `sentry.graphql.enable-specification-error-type-filtering` ([#6092](https://github.com/getsentry/sentry-java/pull/6092)) - Session Replay: Fix `SentryOkHttpInterceptor` hanging forever on responses whose body has no known length, such as Server-Sent Events or a gzipped response, when network details are enabled for the URL ([#6231](https://github.com/getsentry/sentry-java/pull/6231)) - Session Replay: `SentryOkHttpInterceptor` now captures network request and response details only while Session Replay is recording or buffering ([#6486](https://github.com/getsentry/sentry-java/pull/6486)) 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 62a212e6dcc..67eda0dbe86 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 @@ -22,8 +22,8 @@ import io.sentry.time.Deadline; import io.sentry.time.MonotonicTicker; import io.sentry.util.AutoClosableReentrantLock; -import java.util.ArrayList; import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import org.jetbrains.annotations.ApiStatus; @@ -44,19 +44,47 @@ public final class AndroidConnectionStatusProvider private final @NotNull SentryOptions options; private final @NotNull BuildInfoProvider buildInfoProvider; private final @NotNull MonotonicTicker ticker; + + /** + * Copy-on-write, so that observers can be added and removed from any thread without waiting on a + * lock, including from inside a notification. Each notification goes to the observers present + * when it started. + */ private final @NotNull List connectionStatusObservers; + private final @NotNull CellularNetworkTechnologyProvider cellularNetworkTechnologyProvider; private final @Nullable Handler handler; + + /** + * Guards the connection cache: {@link #cachedNetworkCapabilities}, {@link #currentNetwork}, + * {@link #cacheFreshUntil} and {@link #cacheVersion}. Any thread that refreshes a stale cache + * takes it, including the main thread when it captures an event, so it is only held while those + * fields are written, never across a system service call, a log statement or an observer + * notification. + */ private final @NotNull AutoClosableReentrantLock lock = new AutoClosableReentrantLock(); + + /** + * Serializes registering and unregistering the network and telephony callbacks. These are + * synchronous calls into system services that can take a long time, so this lock is separate from + * {@link #lock}. It is only taken on the SDK executor. While holding it, {@link #lock} may be + * acquired briefly, never the other way round. + */ + private final @NotNull AutoClosableReentrantLock registrationLock = + new AutoClosableReentrantLock(); + private volatile @Nullable NetworkCallback networkCallback; private static final @NotNull AutoClosableReentrantLock connectivityManagerLock = new AutoClosableReentrantLock(); private static volatile @Nullable ConnectivityManager connectivityManager; - private static final @NotNull AutoClosableReentrantLock childCallbacksLock = - new AutoClosableReentrantLock(); - private static final @NotNull List childCallbacks = new ArrayList<>(); + /** + * Copy-on-write, so that callbacks are dispatched from a snapshot without holding a lock. The + * callbacks run SDK code such as adding breadcrumbs, which in turn runs user callbacks, while + * other threads, including the main thread, add and remove callbacks. + */ + private static final @NotNull List childCallbacks = new CopyOnWriteArrayList<>(); private static final int[] transports = { NetworkCapabilities.TRANSPORT_WIFI, @@ -70,6 +98,14 @@ public final class AndroidConnectionStatusProvider private volatile @Nullable NetworkCapabilities cachedNetworkCapabilities; private volatile @Nullable Network currentNetwork; private volatile @NotNull Deadline cacheFreshUntil; + + /** + * Incremented, under {@link #lock}, on every write to the cache. A refresh that queries the + * active network outside the lock compares versions before storing its result, so that it does + * not overwrite a newer value written in the meantime, for example by a network callback. + */ + private long cacheVersion; + private static final long CACHE_TTL_MINUTES = 2; private final @NotNull AtomicBoolean isConnected = new AtomicBoolean(false); @@ -94,7 +130,7 @@ public AndroidConnectionStatusProvider( this.ticker = ticker; this.cacheFreshUntil = Deadline.passed(ticker); this.handler = handler; - this.connectionStatusObservers = new ArrayList<>(); + this.connectionStatusObservers = new CopyOnWriteArrayList<>(); this.cellularNetworkTechnologyProvider = new CellularNetworkTechnologyProvider(this.context, options.getLogger(), buildInfoProvider); @@ -200,7 +236,8 @@ private void ensureNetworkCallbackRegistered() { return; // Already registered } - try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { + final boolean registered; + try (final @NotNull ISentryLifecycleToken ignored = registrationLock.acquire()) { if (networkCallback != null) { return; } @@ -215,10 +252,8 @@ public void onAvailable(final @NotNull Network network) { // re-established // otherwise it would've been dispatched on every foreground if (!isConnected.getAndSet(true)) { - try (final @NotNull ISentryLifecycleToken ignored = childCallbacksLock.acquire()) { - for (final @NotNull NetworkCallback cb : childCallbacks) { - cb.onAvailable(network); - } + for (final @NotNull NetworkCallback cb : childCallbacks) { + cb.onAvailable(network); } } } @@ -228,10 +263,8 @@ public void onAvailable(final @NotNull Network network) { public void onUnavailable() { clearCacheAndNotifyObservers(); - try (final @NotNull ISentryLifecycleToken ignored = childCallbacksLock.acquire()) { - for (final @NotNull NetworkCallback cb : childCallbacks) { - cb.onUnavailable(); - } + for (final @NotNull NetworkCallback cb : childCallbacks) { + cb.onUnavailable(); } } @@ -242,10 +275,8 @@ public void onLost(final @NotNull Network network) { } clearCacheAndNotifyObservers(); - try (final @NotNull ISentryLifecycleToken ignored = childCallbacksLock.acquire()) { - for (final @NotNull NetworkCallback cb : childCallbacks) { - cb.onLost(network); - } + for (final @NotNull NetworkCallback cb : childCallbacks) { + cb.onLost(network); } } @@ -256,17 +287,17 @@ private void clearCacheAndNotifyObservers() { cachedNetworkCapabilities = null; currentNetwork = null; cacheFreshUntil = Deadline.after(ticker, CACHE_TTL_MINUTES, TimeUnit.MINUTES); + cacheVersion++; + } - options - .getLogger() - .log(SentryLevel.DEBUG, "Cache cleared - network lost/unavailable"); + options + .getLogger() + .log(SentryLevel.DEBUG, "Cache cleared - network lost/unavailable"); - // Notify all observers with DISCONNECTED status directly - // No need to query ConnectivityManager - we know the network is gone - for (final @NotNull IConnectionStatusObserver observer : - connectionStatusObservers) { - observer.onConnectionStatusChanged(ConnectionStatus.DISCONNECTED); - } + // Notify all observers with DISCONNECTED status directly + // No need to query ConnectivityManager - we know the network is gone + for (final @NotNull IConnectionStatusObserver observer : connectionStatusObservers) { + observer.onConnectionStatusChanged(ConnectionStatus.DISCONNECTED); } } @@ -278,10 +309,8 @@ public void onCapabilitiesChanged( } updateCacheAndNotifyObservers(network, networkCapabilities); - try (final @NotNull ISentryLifecycleToken ignored = childCallbacksLock.acquire()) { - for (final @NotNull NetworkCallback cb : childCallbacks) { - cb.onCapabilitiesChanged(network, networkCapabilities); - } + for (final @NotNull NetworkCallback cb : childCallbacks) { + cb.onCapabilitiesChanged(network, networkCapabilities); } } @@ -295,11 +324,9 @@ private void updateCacheAndNotifyObservers( updateCache(networkCapabilities); final @NotNull ConnectionStatus status = getConnectionStatusFromCache(); - try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { - for (final @NotNull IConnectionStatusObserver observer : - connectionStatusObservers) { - observer.onConnectionStatusChanged(status); - } + for (final @NotNull IConnectionStatusObserver observer : + connectionStatusObservers) { + observer.onConnectionStatusChanged(status); } } } @@ -362,73 +389,96 @@ private boolean hasSignificantTransportChanges( } }; - if (registerNetworkCallback( - context, options.getLogger(), buildInfoProvider, handler, callback)) { + registered = + registerNetworkCallback( + context, options.getLogger(), buildInfoProvider, handler, callback); + if (registered) { networkCallback = callback; // Only start listening once the network callback is registered, because unregistering is // skipped while there is no network callback, which would leave the listener running. cellularNetworkTechnologyProvider.register(); - options.getLogger().log(SentryLevel.DEBUG, "Network callback registered successfully"); - } else { - options.getLogger().log(SentryLevel.WARNING, "Failed to register network callback"); } } + if (registered) { + options.getLogger().log(SentryLevel.DEBUG, "Network callback registered successfully"); + } else { + options.getLogger().log(SentryLevel.WARNING, "Failed to register network callback"); + } } - @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; - cacheFreshUntil = Deadline.after(ticker, CACHE_TTL_MINUTES, TimeUnit.MINUTES); - return; - } - - if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.M) { - cachedNetworkCapabilities = null; - cacheFreshUntil = Deadline.after(ticker, CACHE_TTL_MINUTES, TimeUnit.MINUTES); - 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 - } + /** + * Stores network capabilities in the cache and marks it fresh. + * + * @param networkCapabilities the capabilities to store, or {@code null} to query the active + * network. The query runs without holding {@link #lock}, and its result is discarded if the + * cache was written in the meantime, because that write is at least as recent. + */ + private void updateCache(final @Nullable NetworkCapabilities networkCapabilities) { + if (networkCapabilities != null) { + try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { + cachedNetworkCapabilities = networkCapabilities; + cacheFreshUntil = Deadline.after(ticker, CACHE_TTL_MINUTES, TimeUnit.MINUTES); + cacheVersion++; + } + } else { + final long versionBeforeQuery; + try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { + versionBeforeQuery = cacheVersion; + } + final @Nullable NetworkCapabilities activeNetworkCapabilities = + queryActiveNetworkCapabilities(); + try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { + if (cacheVersion != versionBeforeQuery) { + return; } + cachedNetworkCapabilities = activeNetworkCapabilities; cacheFreshUntil = Deadline.after(ticker, CACHE_TTL_MINUTES, TimeUnit.MINUTES); + cacheVersion++; + } + } + options + .getLogger() + .log( + SentryLevel.DEBUG, + "Cache updated - Status: " + + getConnectionStatusFromCache() + + ", Type: " + + getConnectionTypeFromCache()); + } + + /** + * Queries the capabilities of the active network from the {@link ConnectivityManager}. + * + * @return the capabilities, or {@code null} if they can't be read: without permission, below API + * 23, without an active network, or when the query fails + */ + @SuppressLint({"NewApi", "MissingPermission"}) + private @Nullable NetworkCapabilities queryActiveNetworkCapabilities() { + try { + if (!Permissions.hasPermission(context, Manifest.permission.ACCESS_NETWORK_STATE)) { 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); - cachedNetworkCapabilities = null; - cacheFreshUntil = Deadline.after(ticker, CACHE_TTL_MINUTES, TimeUnit.MINUTES); + .log(SentryLevel.INFO, "No permission (ACCESS_NETWORK_STATE) to check network status."); + return null; + } + + if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.M) { + return null; } + + final ConnectivityManager connectivityManager = + getConnectivityManager(context, options.getLogger()); + if (connectivityManager == null) { + return null; + } + final Network activeNetwork = connectivityManager.getActiveNetwork(); + return activeNetwork != null + ? connectivityManager.getNetworkCapabilities(activeNetwork) + : null; + } catch (Throwable t) { + options.getLogger().log(SentryLevel.WARNING, "Failed to update connection status cache", t); + return null; } } @@ -452,31 +502,36 @@ private boolean isCacheValid() { return getConnectionTypeFromCache(); } + /** + * Adds an observer and makes sure the network callback is registered. + * + *

The observer is added immediately. Registering the network callback calls into system + * services that can be slow, so it is scheduled on the SDK executor instead of making the caller, + * often the main thread during {@code Sentry.init}, wait for it. + * + * @return always {@code true}, because the observer is added even if registering the network + * callback is still pending or fails + */ @Override public boolean addConnectionStatusObserver(final @NotNull IConnectionStatusObserver observer) { - try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { - connectionStatusObservers.add(observer); - } - ensureNetworkCallbackRegistered(); - - // Network callback is already registered during initialization - return networkCallback != null; + connectionStatusObservers.add(observer); + //noinspection Convert2MethodRef + submitSafe(() -> ensureNetworkCallbackRegistered()); + return true; } @Override public void removeConnectionStatusObserver(final @NotNull IConnectionStatusObserver observer) { - try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { - connectionStatusObservers.remove(observer); - // Keep the callback registered for caching even if no observers - } + connectionStatusObservers.remove(observer); + // Keep the callback registered for caching even if no observers } private void unregisterNetworkCallback(final boolean clearObservers) { - try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { - if (clearObservers) { - connectionStatusObservers.clear(); - } + if (clearObservers) { + connectionStatusObservers.clear(); + } + try (final @NotNull ISentryLifecycleToken ignored = registrationLock.acquire()) { final @Nullable NetworkCallback callbackRef = networkCallback; networkCallback = null; @@ -485,9 +540,12 @@ private void unregisterNetworkCallback(final boolean clearObservers) { } cellularNetworkTechnologyProvider.unregister(); // Clear cached state - cachedNetworkCapabilities = null; - currentNetwork = null; - cacheFreshUntil = Deadline.passed(ticker); + try (final @NotNull ISentryLifecycleToken ignoredCache = lock.acquire()) { + cachedNetworkCapabilities = null; + currentNetwork = null; + cacheFreshUntil = Deadline.passed(ticker); + cacheVersion++; + } } options.getLogger().log(SentryLevel.DEBUG, "Network callback unregistered"); } @@ -498,9 +556,7 @@ public void close() { submitSafe( () -> { unregisterNetworkCallback(/* clearObservers= */ true); - try (final @NotNull ISentryLifecycleToken ignored = childCallbacksLock.acquire()) { - childCallbacks.clear(); - } + childCallbacks.clear(); try (final @NotNull ISentryLifecycleToken ignored = connectivityManagerLock.acquire()) { connectivityManager = null; } @@ -508,14 +564,23 @@ public void close() { }); } + /** + * Refreshes the cache, notifies observers and registers the network callback, unless it is + * already registered. + * + *

Whether the callback is registered is checked on the SDK executor, which also runs + * registration and unregistration in submission order. Checking on the calling thread instead + * would skip this work when the app returns to the foreground before the unregistration queued by + * {@link #onBackground()} has run, leaving the callback unregistered while in the foreground. + */ @Override public void onForeground() { - if (networkCallback != null) { - return; - } - submitSafe( () -> { + if (networkCallback != null) { + return; + } + // proactively update cache and notify observers on foreground to ensure connectivity // state is not stale updateCache(null); @@ -525,17 +590,13 @@ public void onForeground() { // onLost is not called retroactively when we registerNetworkCallback (as opposed to // onAvailable), so we have to do it manually for the DISCONNECTED case isConnected.set(false); - try (final @NotNull ISentryLifecycleToken ignored = childCallbacksLock.acquire()) { - for (final @NotNull NetworkCallback cb : childCallbacks) { - //noinspection DataFlowIssue - cb.onLost(null); - } + for (final @NotNull NetworkCallback cb : childCallbacks) { + //noinspection DataFlowIssue + cb.onLost(null); } } - try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { - for (final @NotNull IConnectionStatusObserver observer : connectionStatusObservers) { - observer.onConnectionStatusChanged(status); - } + for (final @NotNull IConnectionStatusObserver observer : connectionStatusObservers) { + observer.onConnectionStatusChanged(status); } // this will ONLY do the necessary parts like registerNetworkCallback and onAvailable, but @@ -545,15 +606,22 @@ public void onForeground() { }); } + /** + * Unregisters the network callback and the cellular technology listener, if the network callback + * is registered. + * + *

Whether the callback is registered is checked on the SDK executor, which also runs + * registration and unregistration in submission order. Checking on the calling thread instead + * would skip unregistering when the app goes to the background while a registration is queued or + * running, leaving the callbacks registered in the background. + */ @Override public void onBackground() { - if (networkCallback == null) { - return; - } - submitSafe( () -> { - //noinspection Convert2MethodRef + if (networkCallback == null) { + return; + } unregisterNetworkCallback(/* clearObservers= */ false); }); } @@ -768,16 +836,12 @@ public static boolean addNetworkCallback( return false; } - try (final @NotNull ISentryLifecycleToken ignored = childCallbacksLock.acquire()) { - childCallbacks.add(networkCallback); - } + childCallbacks.add(networkCallback); return true; } public static void removeNetworkCallback(final @NotNull NetworkCallback networkCallback) { - try (final @NotNull ISentryLifecycleToken ignored = childCallbacksLock.acquire()) { - childCallbacks.remove(networkCallback); - } + childCallbacks.remove(networkCallback); } @SuppressLint({"MissingPermission", "NewApi"}) diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderConcurrencyTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderConcurrencyTest.kt new file mode 100644 index 00000000000..d817e16ec5a --- /dev/null +++ b/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderConcurrencyTest.kt @@ -0,0 +1,679 @@ +package io.sentry.android.core.internal.util + +import android.Manifest +import android.content.Context +import android.content.pm.PackageManager.PERMISSION_GRANTED +import android.net.ConnectivityManager +import android.net.ConnectivityManager.NetworkCallback +import android.net.Network +import android.net.NetworkCapabilities +import android.net.NetworkCapabilities.NET_CAPABILITY_INTERNET +import android.net.NetworkCapabilities.NET_CAPABILITY_VALIDATED +import android.net.NetworkCapabilities.TRANSPORT_CELLULAR +import android.net.NetworkCapabilities.TRANSPORT_WIFI +import android.os.Build +import android.telephony.TelephonyCallback +import android.telephony.TelephonyManager +import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.google.common.truth.Truth.assertThat +import io.sentry.IConnectionStatusProvider.ConnectionStatus +import io.sentry.IConnectionStatusProvider.IConnectionStatusObserver +import io.sentry.ILogger +import io.sentry.ISentryExecutorService +import io.sentry.SentryOptions +import io.sentry.android.core.AppState +import io.sentry.android.core.BuildInfoProvider +import io.sentry.android.core.ContextUtils +import io.sentry.test.DeferredExecutorService +import io.sentry.test.ImmediateExecutorService +import io.sentry.time.TestMonotonicTicker +import java.util.concurrent.Callable +import java.util.concurrent.CountDownLatch +import java.util.concurrent.ExecutorService +import java.util.concurrent.Executors +import java.util.concurrent.Future +import java.util.concurrent.FutureTask +import java.util.concurrent.TimeUnit.SECONDS +import java.util.concurrent.TimeoutException +import kotlin.test.AfterTest +import kotlin.test.Test +import org.junit.runner.RunWith +import org.mockito.MockedStatic +import org.mockito.Mockito.CALLS_REAL_METHODS +import org.mockito.Mockito.mockStatic +import org.mockito.kotlin.any +import org.mockito.kotlin.argumentCaptor +import org.mockito.kotlin.doAnswer +import org.mockito.kotlin.eq +import org.mockito.kotlin.mock +import org.mockito.kotlin.times +import org.mockito.kotlin.verify +import org.mockito.kotlin.verifyNoInteractions +import org.mockito.kotlin.whenever +import org.robolectric.annotation.Config + +/** + * Scenarios in which the SDK executor, network callbacks and callers such as the main thread use + * the provider at the same time. Slow system service calls are stalled with a [Stall], and calls + * that must not wait for them run on a separate caller thread with a bounded wait. + */ +@RunWith(AndroidJUnit4::class) +@Config(sdk = [Build.VERSION_CODES.S]) +class AndroidConnectionStatusProviderConcurrencyTest { + + @AfterTest + fun `tear down`() { + Fixture.closeAll() + } + + // Caller-side calls while the network callback registration is stalled + + @Test + fun `addConnectionStatusObserver does not wait for a stalled network callback registration`() { + val fixture = Fixture.create() + val stall = fixture.stallNetworkCallbackRegistration() + val connectionStatusProvider = fixture.createProvider() + stall.awaitEntered() + val observer = mock() + + val added = fixture.onCallerThread { + connectionStatusProvider.addConnectionStatusObserver(observer) + } + + assertThat(added).isTrue() + assertThat(connectionStatusProvider.statusObservers).containsExactly(observer) + } + + @Test + fun `getConnectionStatus refreshes a stale cache without waiting for a stalled network callback registration`() { + val fixture = Fixture.create() + val stall = fixture.stallNetworkCallbackRegistration() + val connectionStatusProvider = fixture.createProvider() + stall.awaitEntered() + + val status = fixture.onCallerThread { connectionStatusProvider.connectionStatus } + + assertThat(status).isEqualTo(ConnectionStatus.CONNECTED) + } + + @Test + fun `getConnection refreshes a stale cache without waiting for a stalled network callback registration`() { + val fixture = Fixture.create() + val stall = fixture.stallNetworkCallbackRegistration() + val connectionStatusProvider = fixture.createProvider() + stall.awaitEntered() + + val connection = fixture.onCallerThread { connectionStatusProvider.connection } + + assertThat(connection.type).isEqualTo("wifi") + } + + // Caller-side calls while the telephony callback registration is stalled + + @Test + fun `addConnectionStatusObserver does not wait for a stalled telephony callback registration`() { + val fixture = Fixture.create(sdkVersion = Build.VERSION_CODES.S) + val stall = fixture.stallTelephonyCallbackRegistration() + val connectionStatusProvider = fixture.createProvider() + stall.awaitEntered() + val observer = mock() + + val added = fixture.onCallerThread { + connectionStatusProvider.addConnectionStatusObserver(observer) + } + + assertThat(added).isTrue() + assertThat(connectionStatusProvider.statusObservers).containsExactly(observer) + } + + @Test + fun `getConnectionStatus refreshes a stale cache without waiting for a stalled telephony callback registration`() { + val fixture = Fixture.create(sdkVersion = Build.VERSION_CODES.S) + val stall = fixture.stallTelephonyCallbackRegistration() + val connectionStatusProvider = fixture.createProvider() + stall.awaitEntered() + + val status = fixture.onCallerThread { connectionStatusProvider.connectionStatus } + + assertThat(status).isEqualTo(ConnectionStatus.CONNECTED) + } + + // Caller-side calls while unregistering in the background is stalled + + @Test + fun `addConnectionStatusObserver does not wait for a stalled network callback unregistration`() { + val fixture = Fixture.create() + val connectionStatusProvider = fixture.createRegisteredProvider() + val stall = fixture.stallNetworkCallbackUnregistration() + connectionStatusProvider.onBackground() + stall.awaitEntered() + val observer = mock() + + val added = fixture.onCallerThread { + connectionStatusProvider.addConnectionStatusObserver(observer) + } + + assertThat(added).isTrue() + assertThat(connectionStatusProvider.statusObservers).containsExactly(observer) + } + + @Test + fun `getConnectionStatus refreshes a stale cache without waiting for a stalled network callback unregistration`() { + val fixture = Fixture.create() + val connectionStatusProvider = fixture.createRegisteredProvider() + val stall = fixture.stallNetworkCallbackUnregistration() + connectionStatusProvider.onBackground() + stall.awaitEntered() + + val status = fixture.onCallerThread { connectionStatusProvider.connectionStatus } + + assertThat(status).isEqualTo(ConnectionStatus.CONNECTED) + } + + @Test + fun `addConnectionStatusObserver does not wait for a stalled telephony callback unregistration`() { + val fixture = Fixture.create(sdkVersion = Build.VERSION_CODES.S) + val connectionStatusProvider = fixture.createRegisteredProvider() + val stall = fixture.stallTelephonyCallbackUnregistration() + connectionStatusProvider.onBackground() + stall.awaitEntered() + val observer = mock() + + val added = fixture.onCallerThread { + connectionStatusProvider.addConnectionStatusObserver(observer) + } + + assertThat(added).isTrue() + assertThat(connectionStatusProvider.statusObservers).containsExactly(observer) + } + + @Test + fun `getConnectionStatus refreshes a stale cache without waiting for a stalled telephony callback unregistration`() { + val fixture = Fixture.create(sdkVersion = Build.VERSION_CODES.S) + val connectionStatusProvider = fixture.createRegisteredProvider() + val stall = fixture.stallTelephonyCallbackUnregistration() + connectionStatusProvider.onBackground() + stall.awaitEntered() + + val status = fixture.onCallerThread { connectionStatusProvider.connectionStatus } + + assertThat(status).isEqualTo(ConnectionStatus.CONNECTED) + } + + // A stale-cache refresh racing a network callback + + @Test + fun `a stale-cache refresh stores the queried capabilities of the active network`() { + val fixture = Fixture.create() + val connectionStatusProvider = fixture.createRegisteredProvider() + + connectionStatusProvider.connectionStatus + + assertThat(connectionStatusProvider.cachedNetworkCapabilities) + .isSameInstanceAs(fixture.wifiCapabilities) + } + + @Test + fun `capabilities changed while a stale-cache refresh queries the network capabilities are kept`() { + val fixture = Fixture.create() + val connectionStatusProvider = fixture.createRegisteredProvider() + val networkCallback = fixture.captureNetworkCallback() + networkCallback.onAvailable(fixture.network) + val stall = fixture.stallNetworkCapabilitiesQuery() + val refresh = fixture.startOnCallerThread { connectionStatusProvider.connectionStatus } + stall.awaitEntered() + + fixture.onCallerThread { + networkCallback.onCapabilitiesChanged(fixture.network, fixture.cellularCapabilities) + } + stall.release() + refresh.get(TIMEOUT_SECONDS, SECONDS) + + assertThat(connectionStatusProvider.connectionType).isEqualTo("cellular") + } + + @Test + fun `a network lost while a stale-cache refresh queries the network capabilities keeps the cache cleared`() { + val fixture = Fixture.create() + val connectionStatusProvider = fixture.createRegisteredProvider() + val networkCallback = fixture.captureNetworkCallback() + networkCallback.onAvailable(fixture.network) + val stall = fixture.stallNetworkCapabilitiesQuery() + val refresh = fixture.startOnCallerThread { connectionStatusProvider.connectionStatus } + stall.awaitEntered() + + fixture.onCallerThread { networkCallback.onLost(fixture.network) } + stall.release() + refresh.get(TIMEOUT_SECONDS, SECONDS) + + assertThat(connectionStatusProvider.cachedNetworkCapabilities).isNull() + } + + // Observers changing the observers during a notification + + @Test + fun `an observer that removes itself during a notification does not keep the other observers from being notified`() { + val fixture = Fixture.create() + val connectionStatusProvider = fixture.createRegisteredProvider() + val networkCallback = fixture.captureNetworkCallback() + networkCallback.onAvailable(fixture.network) + val selfRemovingObserver = + object : IConnectionStatusObserver { + override fun onConnectionStatusChanged(status: ConnectionStatus) { + connectionStatusProvider.removeConnectionStatusObserver(this) + } + } + val otherObserver = mock() + connectionStatusProvider.addConnectionStatusObserver(selfRemovingObserver) + connectionStatusProvider.addConnectionStatusObserver(otherObserver) + + networkCallback.onLost(fixture.network) + + verify(otherObserver).onConnectionStatusChanged(ConnectionStatus.DISCONNECTED) + assertThat(connectionStatusProvider.statusObservers).containsExactly(otherObserver) + } + + @Test + fun `an observer added during a notification only receives later notifications`() { + val fixture = Fixture.create() + val connectionStatusProvider = fixture.createRegisteredProvider() + val networkCallback = fixture.captureNetworkCallback() + networkCallback.onAvailable(fixture.network) + val addedObserver = mock() + val addingObserver = + object : IConnectionStatusObserver { + override fun onConnectionStatusChanged(status: ConnectionStatus) { + connectionStatusProvider.addConnectionStatusObserver(addedObserver) + } + } + connectionStatusProvider.addConnectionStatusObserver(addingObserver) + + networkCallback.onLost(fixture.network) + + verifyNoInteractions(addedObserver) + assertThat(connectionStatusProvider.statusObservers).contains(addedObserver) + + networkCallback.onUnavailable() + + verify(addedObserver).onConnectionStatusChanged(ConnectionStatus.DISCONNECTED) + } + + // Child network callbacks + + @Test + fun `addNetworkCallback does not wait for a child network callback that is being notified`() { + val fixture = Fixture.create() + fixture.createRegisteredProvider() + val networkCallback = fixture.captureNetworkCallback() + val stall = fixture.newStall() + fixture.addChildNetworkCallback( + object : NetworkCallback() { + override fun onAvailable(network: Network) { + stall.block(Unit) + } + } + ) + fixture.startOnCallerThread { networkCallback.onAvailable(fixture.network) } + stall.awaitEntered() + val childNetworkCallback = mock() + + val added = fixture.onCallerThread { + AndroidConnectionStatusProvider.addNetworkCallback( + fixture.context, + fixture.logger, + fixture.buildInfo, + childNetworkCallback, + ) + } + + assertThat(added).isTrue() + assertThat(AndroidConnectionStatusProvider.getChildCallbacks()).contains(childNetworkCallback) + } + + @Test + fun `removeNetworkCallback does not wait for a child network callback that is being notified`() { + val fixture = Fixture.create() + fixture.createRegisteredProvider() + val networkCallback = fixture.captureNetworkCallback() + val stall = fixture.newStall() + fixture.addChildNetworkCallback( + object : NetworkCallback() { + override fun onAvailable(network: Network) { + stall.block(Unit) + } + } + ) + val childNetworkCallback = mock() + fixture.addChildNetworkCallback(childNetworkCallback) + fixture.startOnCallerThread { networkCallback.onAvailable(fixture.network) } + stall.awaitEntered() + + fixture.onCallerThread { + AndroidConnectionStatusProvider.removeNetworkCallback(childNetworkCallback) + } + + assertThat(AndroidConnectionStatusProvider.getChildCallbacks()) + .doesNotContain(childNetworkCallback) + } + + @Test + fun `a child network callback that removes itself during a notification does not keep the other child network callbacks from being notified`() { + val fixture = Fixture.create() + fixture.createRegisteredProvider() + val networkCallback = fixture.captureNetworkCallback() + fixture.addChildNetworkCallback( + object : NetworkCallback() { + override fun onAvailable(network: Network) { + AndroidConnectionStatusProvider.removeNetworkCallback(this) + } + } + ) + val otherChildNetworkCallback = mock() + fixture.addChildNetworkCallback(otherChildNetworkCallback) + + networkCallback.onAvailable(fixture.network) + + verify(otherChildNetworkCallback).onAvailable(fixture.network) + assertThat(AndroidConnectionStatusProvider.getChildCallbacks()) + .containsExactly(otherChildNetworkCallback) + } + + // Foreground and background transitions queued behind other executor tasks + + @Test + fun `backgrounding while the initial network callback registration is queued unregisters the network callback`() { + val executor = DeferredExecutorService() + val fixture = Fixture.create(executorService = executor) + val connectionStatusProvider = fixture.createProvider() + + connectionStatusProvider.onBackground() + executor.runAll() + + assertThat(connectionStatusProvider.networkCallback).isNull() + verify(fixture.connectivityManager).unregisterNetworkCallback(any()) + } + + @Test + fun `backgrounding while the initial network callback registration is queued unregisters the telephony callback`() { + val executor = DeferredExecutorService() + val fixture = Fixture.create(sdkVersion = Build.VERSION_CODES.S, executorService = executor) + val connectionStatusProvider = fixture.createProvider() + + connectionStatusProvider.onBackground() + executor.runAll() + + verify(fixture.telephonyManager).unregisterTelephonyCallback(any()) + } + + @Test + fun `foregrounding while a background unregistration is queued registers the network callback again`() { + val executor = DeferredExecutorService() + val fixture = Fixture.create(executorService = executor) + val connectionStatusProvider = fixture.createProvider() + executor.runAll() + + connectionStatusProvider.onBackground() + connectionStatusProvider.onForeground() + executor.runAll() + + assertThat(connectionStatusProvider.networkCallback).isNotNull() + verify(fixture.connectivityManager, times(2)).registerDefaultNetworkCallback(any()) + } + + /** Blocks a mocked system service call until released, or until a timeout elapses. */ + private class Stall { + private val entered = CountDownLatch(1) + private val released = CountDownLatch(1) + + fun block(result: T): T { + entered.countDown() + released.await(STALL_TIMEOUT_SECONDS, SECONDS) + return result + } + + fun awaitEntered() { + assertThat(entered.await(TIMEOUT_SECONDS, SECONDS)).isTrue() + } + + fun release() { + released.countDown() + } + } + + /** + * Runs tasks in order on a single thread, like the SDK executor. Mockito's static mocks only + * apply to the thread that creates them, so the thread mocks + * [ContextUtils.isForegroundImportance] itself. + */ + private class SingleThreadExecutorService : ISentryExecutorService { + private val executor: ExecutorService = Executors.newSingleThreadExecutor { + Thread(it, "test-sentry-executor").apply { isDaemon = true } + } + private lateinit var contextUtils: MockedStatic + + init { + executor.submit { contextUtils = mockForegroundImportance() }.get(TIMEOUT_SECONDS, SECONDS) + } + + fun awaitIdle() { + executor.submit {}.get(TIMEOUT_SECONDS, SECONDS) + } + + fun shutdown() { + executor.submit { contextUtils.close() }.get(TIMEOUT_SECONDS, SECONDS) + executor.shutdown() + executor.awaitTermination(TIMEOUT_SECONDS, SECONDS) + } + + override fun submit(runnable: Runnable): Future<*> = executor.submit(runnable) + + override fun submit(callable: Callable): Future = executor.submit(callable) + + override fun schedule(runnable: Runnable, delayMillis: Long): Future<*> = + throw UnsupportedOperationException() + + override fun close(timeoutMillis: Long) {} + + override fun isClosed(): Boolean = executor.isShutdown + } + + private class Fixture + private constructor( + sdkVersion: Int, + private val executorService: ISentryExecutorService, + ) { + val context = mock() + val connectivityManager = mock() + val telephonyManager = mock() + val buildInfo = mock() + val logger = mock() + val network = mock() + val wifiCapabilities = capabilities(TRANSPORT_WIFI) + val cellularCapabilities = capabilities(TRANSPORT_CELLULAR) + private val stalls = mutableListOf() + private val callerThreads = mutableListOf() + + private val options = SentryOptions() + private val contextUtils = mockForegroundImportance() + private var connectionStatusProvider: AndroidConnectionStatusProvider? = null + + init { + whenever(context.getSystemService(Context.CONNECTIVITY_SERVICE)) + .thenReturn(connectivityManager) + whenever(context.getSystemService(Context.TELEPHONY_SERVICE)).thenReturn(telephonyManager) + whenever(context.checkPermission(eq(Manifest.permission.ACCESS_NETWORK_STATE), any(), any())) + .thenReturn(PERMISSION_GRANTED) + whenever(buildInfo.sdkInfoVersion).thenReturn(sdkVersion) + whenever(connectivityManager.activeNetwork).thenReturn(network) + whenever(connectivityManager.getNetworkCapabilities(any())).thenReturn(wifiCapabilities) + + // Another suite can leave its connectivity manager and child callbacks in the static fields. + AndroidConnectionStatusProvider::class + .java + .getDeclaredField("connectivityManager") + .apply { isAccessible = true } + .set(null, null) + AndroidConnectionStatusProvider.getChildCallbacks().clear() + + options.setLogger(logger) + options.executorService = executorService + + AppState.getInstance().resetInstance() + AppState.getInstance().registerLifecycleObserver(options) + } + + fun createProvider(): AndroidConnectionStatusProvider = + AndroidConnectionStatusProvider(context, options, buildInfo, TestMonotonicTicker()).also { + connectionStatusProvider = it + } + + /** Creates a provider and waits until its network callback registration has run. */ + fun createRegisteredProvider(): AndroidConnectionStatusProvider = + createProvider().also { awaitExecutorIdle() } + + fun awaitExecutorIdle() { + (executorService as SingleThreadExecutorService).awaitIdle() + } + + fun captureNetworkCallback(): NetworkCallback { + val captor = argumentCaptor() + verify(connectivityManager).registerDefaultNetworkCallback(captor.capture()) + return captor.lastValue + } + + fun addChildNetworkCallback(networkCallback: NetworkCallback) { + AndroidConnectionStatusProvider.addNetworkCallback( + context, + logger, + buildInfo, + networkCallback, + ) + } + + fun stallNetworkCallbackRegistration(): Stall = + newStall().also { stall -> + doAnswer { stall.block(null) } + .whenever(connectivityManager) + .registerDefaultNetworkCallback(any()) + } + + fun stallNetworkCallbackUnregistration(): Stall = + newStall().also { stall -> + doAnswer { stall.block(null) } + .whenever(connectivityManager) + .unregisterNetworkCallback(any()) + } + + fun stallTelephonyCallbackRegistration(): Stall = + newStall().also { stall -> + doAnswer { stall.block(null) } + .whenever(telephonyManager) + .registerTelephonyCallback(any(), any()) + } + + fun stallTelephonyCallbackUnregistration(): Stall = + newStall().also { stall -> + doAnswer { stall.block(null) } + .whenever(telephonyManager) + .unregisterTelephonyCallback(any()) + } + + fun stallNetworkCapabilitiesQuery(): Stall = + newStall().also { stall -> + whenever(connectivityManager.getNetworkCapabilities(any())).thenAnswer { + stall.block(wifiCapabilities) + } + } + + fun newStall(): Stall = Stall().also { stalls += it } + + /** Starts [action] on a new thread, standing in for a caller such as the main thread. */ + fun startOnCallerThread(action: () -> T): FutureTask = + FutureTask(action).also { task -> + callerThreads += Thread(task, "test-caller").apply { isDaemon = true }.also { it.start() } + } + + /** Runs [action] on a new caller thread and fails if it does not complete in time. */ + fun onCallerThread(action: () -> T): T { + val task = startOnCallerThread(action) + try { + return task.get(TIMEOUT_SECONDS, SECONDS) + } catch (e: TimeoutException) { + throw AssertionError("The call did not complete within $TIMEOUT_SECONDS seconds", e) + } + } + + fun close() { + stalls.forEach { it.release() } + // A caller that is still running after close() would cache this test's connectivity + // manager in the static field that close() resets, and leak it into the next test. + callerThreads.forEach { it.join(SECONDS.toMillis(TIMEOUT_SECONDS)) } + connectionStatusProvider?.close() + when (executorService) { + is SingleThreadExecutorService -> executorService.shutdown() + is DeferredExecutorService -> executorService.runAll() + } + contextUtils.close() + AppState.getInstance().unregisterLifecycleObserver() + } + + companion object { + private val created = mutableListOf() + + fun create( + sdkVersion: Int = Build.VERSION_CODES.N, + executorService: ISentryExecutorService = SingleThreadExecutorService(), + ): Fixture { + resetStaticState() + return Fixture(sdkVersion, executorService).also { created += it } + } + + fun closeAll() { + created.forEach { it.close() } + created.clear() + } + + /** + * Clears the cached ConnectivityManager and the child network callbacks, which are static and + * may have been left behind by other suites, by closing a provider that registers nothing. + */ + private fun resetStaticState() { + val resetOptions = SentryOptions().apply { executorService = ImmediateExecutorService() } + val foregroundImportance = + mockStatic(ContextUtils::class.java, CALLS_REAL_METHODS).also { + it.`when` { ContextUtils.isForegroundImportance() }.thenReturn(false) + } + try { + AndroidConnectionStatusProvider(mock(), resetOptions, mock(), TestMonotonicTicker()) + .close() + } finally { + foregroundImportance.close() + } + } + + private fun capabilities(transport: Int): NetworkCapabilities { + val capabilities = mock() + whenever(capabilities.hasCapability(NET_CAPABILITY_INTERNET)).thenReturn(true) + whenever(capabilities.hasCapability(NET_CAPABILITY_VALIDATED)).thenReturn(true) + whenever(capabilities.hasTransport(transport)).thenReturn(true) + return capabilities + } + } + } + + private companion object { + /** Bounds every wait, so a regression fails the test instead of hanging the build. */ + const val TIMEOUT_SECONDS = 5L + + /** + * Bounds a stalled call, which the test releases itself. Much longer than [TIMEOUT_SECONDS], so + * that a caller waiting for the stalled call times out before the stall gives up. + */ + const val STALL_TIMEOUT_SECONDS = 30L + + fun mockForegroundImportance(): MockedStatic = + mockStatic(ContextUtils::class.java, CALLS_REAL_METHODS).also { + it.`when` { ContextUtils.isForegroundImportance() }.thenReturn(true) + } + } +} 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 56a2a12928c..d2e9c509ece 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 @@ -19,6 +19,7 @@ import android.os.Handler import android.os.Looper import android.telephony.TelephonyManager import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.google.common.truth.Truth.assertThat import io.sentry.IConnectionStatusProvider import io.sentry.ILogger import io.sentry.SentryOptions @@ -379,6 +380,23 @@ class AndroidConnectionStatusProviderTest { assertTrue(connectionStatusProvider.statusObservers.isEmpty()) } + @Test + fun `addConnectionStatusObserver adds the observer and returns true when the network callback cannot be registered`() { + whenever( + contextMock.checkPermission(eq(Manifest.permission.ACCESS_NETWORK_STATE), any(), any()) + ) + .thenReturn(PERMISSION_DENIED) + val provider = AndroidConnectionStatusProvider(contextMock, options, buildInfo, ticker) + val observer = IConnectionStatusProvider.IConnectionStatusObserver {} + + val added = provider.addConnectionStatusObserver(observer) + + assertThat(added).isTrue() + assertThat(provider.statusObservers).containsExactly(observer) + assertThat(provider.networkCallback).isNull() + provider.close() + } + @Test fun `an unpopulated cache is not treated as fresh shortly after boot`() { whenever(networkInfo.isConnected).thenReturn(true) From 51a88a903f41ddc7da61d4e5e75fea275ed0a461 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Mon, 9 Nov 2026 12:54:12 +0100 Subject: [PATCH 02/10] ref(android): Hold the connection cache in an immutable snapshot The cache was four fields (capabilities, network, freshness deadline and a version counter) guarded by a lock, written in four places that each had to remember to bump the version. onAvailable wrote the network without the lock, and the status lookup read the capabilities twice, so a concurrent unregister could make it report DISCONNECTED instead of querying. Replace them with an immutable ConnectionSnapshot in an AtomicReference. Writes swap in a new snapshot with compare-and-set, a refresh stores its result only if the snapshot is unchanged, and readers derive status and type from one snapshot. The capabilities that decide a significant change are now per instance instead of a static array rewritten by every constructor. Refs #6509 --- .../util/AndroidConnectionStatusProvider.java | 283 ++++++++++-------- ...ConnectionStatusProviderConcurrencyTest.kt | 33 ++ 2 files changed, 189 insertions(+), 127 deletions(-) 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 67eda0dbe86..6c1e69ee809 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 @@ -26,6 +26,7 @@ import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -56,19 +57,16 @@ public final class AndroidConnectionStatusProvider private final @Nullable Handler handler; /** - * Guards the connection cache: {@link #cachedNetworkCapabilities}, {@link #currentNetwork}, - * {@link #cacheFreshUntil} and {@link #cacheVersion}. Any thread that refreshes a stale cache - * takes it, including the main thread when it captures an event, so it is only held while those - * fields are written, never across a system service call, a log statement or an observer - * notification. + * What the provider currently knows about the default network. Replaced as a whole, never + * modified, so readers on any thread, including the main thread when it captures an event, see a + * consistent value without taking a lock. */ - private final @NotNull AutoClosableReentrantLock lock = new AutoClosableReentrantLock(); + private final @NotNull AtomicReference snapshot; /** * Serializes registering and unregistering the network and telephony callbacks. These are - * synchronous calls into system services that can take a long time, so this lock is separate from - * {@link #lock}. It is only taken on the SDK executor. While holding it, {@link #lock} may be - * acquired briefly, never the other way round. + * synchronous calls into system services that can take a long time, so the lock is only taken on + * the SDK executor, and nothing that callers wait for is done while it is held. */ private final @NotNull AutoClosableReentrantLock registrationLock = new AutoClosableReentrantLock(); @@ -93,18 +91,10 @@ public final class AndroidConnectionStatusProvider NetworkCapabilities.TRANSPORT_BLUETOOTH }; - private static final int[] capabilities = new int[2]; - - private volatile @Nullable NetworkCapabilities cachedNetworkCapabilities; - private volatile @Nullable Network currentNetwork; - private volatile @NotNull Deadline cacheFreshUntil; - /** - * Incremented, under {@link #lock}, on every write to the cache. A refresh that queries the - * active network outside the lock compares versions before storing its result, so that it does - * not overwrite a newer value written in the meantime, for example by a network callback. + * The capabilities whose change affects the connection status, which depends on the API level. */ - private long cacheVersion; + private final int[] capabilityIds; private static final long CACHE_TTL_MINUTES = 2; private final @NotNull AtomicBoolean isConnected = new AtomicBoolean(false); @@ -128,16 +118,19 @@ public AndroidConnectionStatusProvider( this.options = options; this.buildInfoProvider = buildInfoProvider; this.ticker = ticker; - this.cacheFreshUntil = Deadline.passed(ticker); + this.snapshot = new AtomicReference<>(ConnectionSnapshot.unknown(ticker)); this.handler = handler; this.connectionStatusObservers = new CopyOnWriteArrayList<>(); this.cellularNetworkTechnologyProvider = new CellularNetworkTechnologyProvider(this.context, options.getLogger(), buildInfoProvider); - capabilities[0] = NetworkCapabilities.NET_CAPABILITY_INTERNET; - if (buildInfoProvider.getSdkInfoVersion() >= Build.VERSION_CODES.M) { - capabilities[1] = NetworkCapabilities.NET_CAPABILITY_VALIDATED; - } + this.capabilityIds = + buildInfoProvider.getSdkInfoVersion() >= Build.VERSION_CODES.M + ? new int[] { + NetworkCapabilities.NET_CAPABILITY_INTERNET, + NetworkCapabilities.NET_CAPABILITY_VALIDATED + } + : new int[] {NetworkCapabilities.NET_CAPABILITY_INTERNET}; // Register network callback immediately for caching //noinspection Convert2MethodRef @@ -180,10 +173,14 @@ private boolean isNetworkEffectivelyConnected( return false; } - /** Get connection status from cached NetworkCapabilities or fallback to legacy method. */ - private @NotNull ConnectionStatus getConnectionStatusFromCache() { - if (cachedNetworkCapabilities != null) { - return isNetworkEffectivelyConnected(cachedNetworkCapabilities) + /** + * The connection status of a snapshot, or of the legacy {@link ConnectivityManager} query when + * the snapshot has no capabilities. + */ + private @NotNull ConnectionStatus statusOf(final @NotNull ConnectionSnapshot snapshot) { + final @Nullable NetworkCapabilities capabilities = snapshot.capabilities; + if (capabilities != null) { + return isNetworkEffectivelyConnected(capabilities) ? ConnectionStatus.CONNECTED : ConnectionStatus.DISCONNECTED; } @@ -198,9 +195,12 @@ private boolean isNetworkEffectivelyConnected( return ConnectionStatus.UNKNOWN; } - /** Get connection type from cached NetworkCapabilities or fallback to legacy method. */ - private @Nullable String getConnectionTypeFromCache() { - final NetworkCapabilities capabilities = cachedNetworkCapabilities; + /** + * The connection type of a snapshot, or of the legacy {@link ConnectivityManager} query when the + * snapshot has no capabilities. + */ + private @Nullable String typeOf(final @NotNull ConnectionSnapshot snapshot) { + final @Nullable NetworkCapabilities capabilities = snapshot.capabilities; if (capabilities != null) { return getConnectionType(capabilities); } @@ -212,14 +212,11 @@ private boolean isNetworkEffectivelyConnected( /** * The connection type and, for cellular connections, the generation of the network technology. * - *

Both are derived from one cache read, so they always describe the same network instead of - * straddling a connectivity change. + *

The type is derived from a single snapshot, so it can't mix two networks across a + * connectivity change. */ public @NotNull NetworkConnection getConnection() { - if (!isCacheValid()) { - updateCache(null); - } - final @Nullable String connectionType = getConnectionTypeFromCache(); + final @Nullable String connectionType = typeOf(freshSnapshot()); if (!"cellular".equals(connectionType)) { return new NetworkConnection(connectionType, null); } @@ -246,7 +243,10 @@ private void ensureNetworkCallbackRegistered() { new NetworkCallback() { @Override public void onAvailable(final @NotNull Network network) { - currentNetwork = network; + @NotNull ConnectionSnapshot current; + do { + current = snapshot.get(); + } while (!snapshot.compareAndSet(current, current.withNetwork(network))); // have to only dispatch this on first registration + when the connection got // re-established @@ -270,7 +270,7 @@ public void onUnavailable() { @Override public void onLost(final @NotNull Network network) { - if (!network.equals(currentNetwork)) { + if (!network.equals(snapshot.get().network)) { return; } clearCacheAndNotifyObservers(); @@ -282,13 +282,7 @@ public void onLost(final @NotNull Network network) { private void clearCacheAndNotifyObservers() { isConnected.set(false); - // Clear cached capabilities and network reference atomically - try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { - cachedNetworkCapabilities = null; - currentNetwork = null; - cacheFreshUntil = Deadline.after(ticker, CACHE_TTL_MINUTES, TimeUnit.MINUTES); - cacheVersion++; - } + snapshot.set(ConnectionSnapshot.lost(ticker)); options .getLogger() @@ -304,30 +298,38 @@ private void clearCacheAndNotifyObservers() { @Override public void onCapabilitiesChanged( @NonNull Network network, @NonNull NetworkCapabilities networkCapabilities) { - if (!network.equals(currentNetwork)) { + if (!network.equals(snapshot.get().network)) { return; } - updateCacheAndNotifyObservers(network, networkCapabilities); + updateCacheAndNotifyObservers(networkCapabilities); for (final @NotNull NetworkCallback cb : childCallbacks) { cb.onCapabilitiesChanged(network, networkCapabilities); } } + /** + * Stores the capabilities and notifies observers, but only if they differ from the + * stored ones in a way that changes the connection status or type. + */ private void updateCacheAndNotifyObservers( - @Nullable Network network, @Nullable NetworkCapabilities networkCapabilities) { - // Check if this change is meaningful before notifying observers - final boolean shouldUpdate = isSignificantChange(networkCapabilities); - - // Only notify observers if something meaningful changed - if (shouldUpdate) { - updateCache(networkCapabilities); - - final @NotNull ConnectionStatus status = getConnectionStatusFromCache(); - for (final @NotNull IConnectionStatusObserver observer : - connectionStatusObservers) { - observer.onConnectionStatusChanged(status); + final @NotNull NetworkCapabilities networkCapabilities) { + @NotNull ConnectionSnapshot current; + @NotNull ConnectionSnapshot updated; + do { + current = snapshot.get(); + if (!isSignificantChange(current.capabilities, networkCapabilities)) { + return; } + updated = + current.withCapabilities( + networkCapabilities, + Deadline.after(ticker, CACHE_TTL_MINUTES, TimeUnit.MINUTES)); + } while (!snapshot.compareAndSet(current, updated)); + + final @NotNull ConnectionStatus status = statusOf(updated); + for (final @NotNull IConnectionStatusObserver observer : connectionStatusObservers) { + observer.onConnectionStatusChanged(status); } } @@ -335,19 +337,14 @@ private void updateCacheAndNotifyObservers( * Check if NetworkCapabilities change is significant for our observers. Only notify for * changes that affect connectivity status or connection type. */ - private boolean isSignificantChange(@Nullable NetworkCapabilities newCapabilities) { - final NetworkCapabilities oldCapabilities = cachedNetworkCapabilities; - - // Always significant if transitioning between null and non-null - if ((oldCapabilities == null) != (newCapabilities == null)) { + private boolean isSignificantChange( + final @Nullable NetworkCapabilities oldCapabilities, + final @NotNull NetworkCapabilities newCapabilities) { + // Always significant when capabilities first become available + if (oldCapabilities == null) { return true; } - // If both null, no change - if (oldCapabilities == null && newCapabilities == null) { - return false; - } - // Check significant capability changes if (hasSignificantCapabilityChanges(oldCapabilities, newCapabilities)) { return true; @@ -365,9 +362,8 @@ private boolean isSignificantChange(@Nullable NetworkCapabilities newCapabilitie private boolean hasSignificantCapabilityChanges( @NotNull NetworkCapabilities old, @NotNull NetworkCapabilities new_) { // Check capabilities we care about for connectivity determination - for (int capability : capabilities) { - if (capability != 0 - && old.hasCapability(capability) != new_.hasCapability(capability)) { + for (int capabilityId : capabilityIds) { + if (old.hasCapability(capabilityId) != new_.hasCapability(capabilityId)) { return true; } } @@ -406,45 +402,35 @@ private boolean hasSignificantTransportChanges( } } + /** The current snapshot, refreshed first if it is stale. */ + private @NotNull ConnectionSnapshot freshSnapshot() { + final @NotNull ConnectionSnapshot current = snapshot.get(); + return current.isFresh() ? current : refresh(current); + } + /** - * Stores network capabilities in the cache and marks it fresh. + * Queries the active network and stores its capabilities, unless the snapshot changed during the + * query. * - * @param networkCapabilities the capabilities to store, or {@code null} to query the active - * network. The query runs without holding {@link #lock}, and its result is discarded if the - * cache was written in the meantime, because that write is at least as recent. + * @param before the snapshot the refresh replaces + * @return the stored snapshot, or, if another thread such as a network callback replaced {@code + * before} during the query, that newer snapshot, which may still be stale */ - private void updateCache(final @Nullable NetworkCapabilities networkCapabilities) { - if (networkCapabilities != null) { - try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { - cachedNetworkCapabilities = networkCapabilities; - cacheFreshUntil = Deadline.after(ticker, CACHE_TTL_MINUTES, TimeUnit.MINUTES); - cacheVersion++; - } - } else { - final long versionBeforeQuery; - try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { - versionBeforeQuery = cacheVersion; - } - final @Nullable NetworkCapabilities activeNetworkCapabilities = - queryActiveNetworkCapabilities(); - try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { - if (cacheVersion != versionBeforeQuery) { - return; - } - cachedNetworkCapabilities = activeNetworkCapabilities; - cacheFreshUntil = Deadline.after(ticker, CACHE_TTL_MINUTES, TimeUnit.MINUTES); - cacheVersion++; - } + private @NotNull ConnectionSnapshot refresh(final @NotNull ConnectionSnapshot before) { + final @NotNull ConnectionSnapshot refreshed = + before.withCapabilities( + queryActiveNetworkCapabilities(), + Deadline.after(ticker, CACHE_TTL_MINUTES, TimeUnit.MINUTES)); + if (!snapshot.compareAndSet(before, refreshed)) { + return snapshot.get(); } options .getLogger() .log( SentryLevel.DEBUG, - "Cache updated - Status: " - + getConnectionStatusFromCache() - + ", Type: " - + getConnectionTypeFromCache()); + "Cache updated - Status: " + statusOf(refreshed) + ", Type: " + typeOf(refreshed)); + return refreshed; } /** @@ -482,24 +468,14 @@ private void updateCache(final @Nullable NetworkCapabilities networkCapabilities } } - private boolean isCacheValid() { - return !cacheFreshUntil.hasPassed(); - } - @Override public @NotNull ConnectionStatus getConnectionStatus() { - if (!isCacheValid()) { - updateCache(null); - } - return getConnectionStatusFromCache(); + return statusOf(freshSnapshot()); } @Override public @Nullable String getConnectionType() { - if (!isCacheValid()) { - updateCache(null); - } - return getConnectionTypeFromCache(); + return typeOf(freshSnapshot()); } /** @@ -539,13 +515,7 @@ private void unregisterNetworkCallback(final boolean clearObservers) { unregisterNetworkCallback(context, options.getLogger(), callbackRef); } cellularNetworkTechnologyProvider.unregister(); - // Clear cached state - try (final @NotNull ISentryLifecycleToken ignoredCache = lock.acquire()) { - cachedNetworkCapabilities = null; - currentNetwork = null; - cacheFreshUntil = Deadline.passed(ticker); - cacheVersion++; - } + snapshot.set(ConnectionSnapshot.unknown(ticker)); } options.getLogger().log(SentryLevel.DEBUG, "Network callback unregistered"); } @@ -583,9 +553,7 @@ public void onForeground() { // proactively update cache and notify observers on foreground to ensure connectivity // state is not stale - updateCache(null); - - final @NotNull ConnectionStatus status = getConnectionStatusFromCache(); + final @NotNull ConnectionStatus status = statusOf(refresh(snapshot.get())); if (status == ConnectionStatus.DISCONNECTED) { // onLost is not called retroactively when we registerNetworkCallback (as opposed to // onAvailable), so we have to do it manually for the DISCONNECTED case @@ -600,7 +568,8 @@ public void onForeground() { } // this will ONLY do the necessary parts like registerNetworkCallback and onAvailable, but - // it won't updateCache and notify observes because we just did it above and the cached + // it won't refresh the cache and notify observers because we just did it above and the + // cached // capabilities will be the same ensureNetworkCallbackRegistered(); }); @@ -635,7 +604,7 @@ public void onBackground() { @TestOnly @Nullable public NetworkCapabilities getCachedNetworkCapabilities() { - return cachedNetworkCapabilities; + return snapshot.get().capabilities; } /** @@ -911,6 +880,66 @@ public static List getChildCallbacks() { return childCallbacks; } + /** + * An immutable view of the default network: which network it is, its capabilities, and until when + * they can be used without querying the {@link ConnectivityManager} again. + * + *

Each change replaces the whole snapshot, so a reader that reads it once gets values that + * belong together, and a refresh can detect with a compare-and-set that a network callback + * replaced it during the query. + */ + private static final class ConnectionSnapshot { + /** The default network reported by the network callback, or {@code null} if none is known. */ + final @Nullable Network network; + + /** + * The capabilities of the default network, or {@code null} if they are unknown or there is no + * default network. Readers then fall back to the legacy {@link ConnectivityManager} query. + */ + final @Nullable NetworkCapabilities capabilities; + + final @NotNull Deadline freshUntil; + + private ConnectionSnapshot( + final @Nullable Network network, + final @Nullable NetworkCapabilities capabilities, + final @NotNull Deadline freshUntil) { + this.network = network; + this.capabilities = capabilities; + this.freshUntil = freshUntil; + } + + /** Nothing is known, so the next read queries the active network. */ + static @NotNull ConnectionSnapshot unknown(final @NotNull MonotonicTicker ticker) { + return new ConnectionSnapshot(null, null, Deadline.passed(ticker)); + } + + /** + * The network callback reported that the default network is gone. The snapshot is not refreshed + * until the cache time-to-live has elapsed, but it has no capabilities, so reading the status + * or type still falls back to the legacy {@link ConnectivityManager} query on every read. + */ + static @NotNull ConnectionSnapshot lost(final @NotNull MonotonicTicker ticker) { + return new ConnectionSnapshot( + null, null, Deadline.after(ticker, CACHE_TTL_MINUTES, TimeUnit.MINUTES)); + } + + boolean isFresh() { + return !freshUntil.hasPassed(); + } + + @NotNull + ConnectionSnapshot withNetwork(final @NotNull Network network) { + return new ConnectionSnapshot(network, capabilities, freshUntil); + } + + @NotNull + ConnectionSnapshot withCapabilities( + final @Nullable NetworkCapabilities capabilities, final @NotNull Deadline freshUntil) { + return new ConnectionSnapshot(network, capabilities, freshUntil); + } + } + private void submitSafe(@NotNull Runnable r) { try { options.getExecutorService().submit(r); diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderConcurrencyTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderConcurrencyTest.kt index d817e16ec5a..329cb20176b 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderConcurrencyTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderConcurrencyTest.kt @@ -126,6 +126,20 @@ class AndroidConnectionStatusProviderConcurrencyTest { assertThat(connectionStatusProvider.statusObservers).containsExactly(observer) } + @Test + fun `removeConnectionStatusObserver does not wait for a stalled telephony callback registration`() { + val fixture = Fixture.create(sdkVersion = Build.VERSION_CODES.S) + val stall = fixture.stallTelephonyCallbackRegistration() + val connectionStatusProvider = fixture.createProvider() + stall.awaitEntered() + val observer = mock() + connectionStatusProvider.addConnectionStatusObserver(observer) + + fixture.onCallerThread { connectionStatusProvider.removeConnectionStatusObserver(observer) } + + assertThat(connectionStatusProvider.statusObservers).isEmpty() + } + @Test fun `getConnectionStatus refreshes a stale cache without waiting for a stalled telephony callback registration`() { val fixture = Fixture.create(sdkVersion = Build.VERSION_CODES.S) @@ -249,6 +263,25 @@ class AndroidConnectionStatusProviderConcurrencyTest { assertThat(connectionStatusProvider.cachedNetworkCapabilities).isNull() } + @Test + fun `a network that becomes available while a stale-cache refresh queries the network capabilities stays the current network`() { + val fixture = Fixture.create() + val connectionStatusProvider = fixture.createRegisteredProvider() + val networkCallback = fixture.captureNetworkCallback() + val observer = mock() + connectionStatusProvider.addConnectionStatusObserver(observer) + val stall = fixture.stallNetworkCapabilitiesQuery() + val refresh = fixture.startOnCallerThread { connectionStatusProvider.connectionStatus } + stall.awaitEntered() + + fixture.onCallerThread { networkCallback.onAvailable(fixture.network) } + stall.release() + refresh.get(TIMEOUT_SECONDS, SECONDS) + networkCallback.onLost(fixture.network) + + verify(observer).onConnectionStatusChanged(ConnectionStatus.DISCONNECTED) + } + // Observers changing the observers during a notification @Test From 098064c37649d1107aec9f9b9dbbd93e1cd254e4 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Mon, 9 Nov 2026 13:07:40 +0100 Subject: [PATCH 03/10] fix(android): Don't register connection callbacks again after close Registration now runs on the SDK executor, so a registration queued after close() ran after its cleanup and registered the network and cellular callbacks again, with nothing left to unregister them. close() now sets a flag first that queued registration and foreground work check. Refs #6509 --- .../util/AndroidConnectionStatusProvider.java | 22 +++- ...ConnectionStatusProviderConcurrencyTest.kt | 123 ++++++++++++++++++ 2 files changed, 142 insertions(+), 3 deletions(-) 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 6c1e69ee809..4d458cbef47 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 @@ -73,6 +73,13 @@ public final class AndroidConnectionStatusProvider private volatile @Nullable NetworkCallback networkCallback; + /** + * Set by {@link #close()} on the calling thread, before the unregistration it schedules runs. + * Registration and foreground refreshes queued on the SDK executor after that unregistration + * check it, so they don't register the callbacks again on a closed provider. + */ + private volatile boolean closed; + private static final @NotNull AutoClosableReentrantLock connectivityManagerLock = new AutoClosableReentrantLock(); private static volatile @Nullable ConnectivityManager connectivityManager; @@ -225,6 +232,10 @@ private boolean isNetworkEffectivelyConnected( } private void ensureNetworkCallbackRegistered() { + if (closed) { + return; + } + if (!ContextUtils.isForegroundImportance()) { return; } @@ -235,7 +246,9 @@ private void ensureNetworkCallbackRegistered() { final boolean registered; try (final @NotNull ISentryLifecycleToken ignored = registrationLock.acquire()) { - if (networkCallback != null) { + // Checked again under the lock in case close() ran in the meantime: its unregistration takes + // this lock too, so either it sees the registration and undoes it, or this sees the flag. + if (closed || networkCallback != null) { return; } @@ -491,6 +504,8 @@ private boolean hasSignificantTransportChanges( @Override public boolean addConnectionStatusObserver(final @NotNull IConnectionStatusObserver observer) { connectionStatusObservers.add(observer); + // The executor drops tasks when its queue is full, which would skip this registration. That is + // unlikely, and onForeground() registers the callback again the next time the app is shown. //noinspection Convert2MethodRef submitSafe(() -> ensureNetworkCallbackRegistered()); return true; @@ -523,6 +538,7 @@ private void unregisterNetworkCallback(final boolean clearObservers) { /** Clean up resources - should be called when the provider is no longer needed */ @Override public void close() { + closed = true; submitSafe( () -> { unregisterNetworkCallback(/* clearObservers= */ true); @@ -536,7 +552,7 @@ public void close() { /** * Refreshes the cache, notifies observers and registers the network callback, unless it is - * already registered. + * already registered or the provider is closed. * *

Whether the callback is registered is checked on the SDK executor, which also runs * registration and unregistration in submission order. Checking on the calling thread instead @@ -547,7 +563,7 @@ public void close() { public void onForeground() { submitSafe( () -> { - if (networkCallback != null) { + if (closed || networkCallback != null) { return; } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderConcurrencyTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderConcurrencyTest.kt index 329cb20176b..80d34d06a11 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderConcurrencyTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderConcurrencyTest.kt @@ -42,10 +42,12 @@ import org.mockito.MockedStatic import org.mockito.Mockito.CALLS_REAL_METHODS import org.mockito.Mockito.mockStatic import org.mockito.kotlin.any +import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.argumentCaptor import org.mockito.kotlin.doAnswer import org.mockito.kotlin.eq import org.mockito.kotlin.mock +import org.mockito.kotlin.never import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.verifyNoInteractions @@ -453,6 +455,127 @@ class AndroidConnectionStatusProviderConcurrencyTest { verify(fixture.connectivityManager, times(2)).registerDefaultNetworkCallback(any()) } + @Test + fun `adding an observer while a background unregistration is queued registers the network callback again`() { + val executor = DeferredExecutorService() + val fixture = Fixture.create(executorService = executor) + val connectionStatusProvider = fixture.createProvider() + executor.runAll() + + connectionStatusProvider.onBackground() + connectionStatusProvider.addConnectionStatusObserver(mock()) + executor.runAll() + + assertThat(connectionStatusProvider.networkCallback).isNotNull() + verify(fixture.connectivityManager, times(2)).registerDefaultNetworkCallback(any()) + } + + // Registrations queued around close + + @Test + fun `closing after the network callback registration ran unregisters the network callback`() { + val executor = DeferredExecutorService() + val fixture = Fixture.create(executorService = executor) + val connectionStatusProvider = fixture.createProvider() + executor.runAll() + + connectionStatusProvider.close() + executor.runAll() + + assertThat(connectionStatusProvider.networkCallback).isNull() + verify(fixture.connectivityManager).unregisterNetworkCallback(any()) + } + + @Test + fun `closing while the network callback registration is running unregisters the network callback`() { + val fixture = Fixture.create() + val stall = fixture.stallNetworkCallbackRegistration() + val connectionStatusProvider = fixture.createProvider() + stall.awaitEntered() + + connectionStatusProvider.close() + stall.release() + fixture.awaitExecutorIdle() + + assertThat(connectionStatusProvider.networkCallback).isNull() + verify(fixture.connectivityManager).unregisterNetworkCallback(any()) + } + + @Test + fun `closing while the initial network callback registration is queued does not register the network callback`() { + val executor = DeferredExecutorService() + val fixture = Fixture.create(executorService = executor) + val connectionStatusProvider = fixture.createProvider() + + connectionStatusProvider.close() + executor.runAll() + + assertThat(connectionStatusProvider.networkCallback).isNull() + verify(fixture.connectivityManager, never()).registerDefaultNetworkCallback(any()) + } + + @Test + fun `adding an observer after close does not register the network callback again`() { + val executor = DeferredExecutorService() + val fixture = Fixture.create(executorService = executor) + val connectionStatusProvider = fixture.createProvider() + executor.runAll() + + connectionStatusProvider.close() + connectionStatusProvider.addConnectionStatusObserver(mock()) + executor.runAll() + + assertThat(connectionStatusProvider.networkCallback).isNull() + verify(fixture.connectivityManager, times(1)).registerDefaultNetworkCallback(any()) + } + + @Test + fun `adding an observer after close does not register the telephony callback again`() { + val executor = DeferredExecutorService() + val fixture = Fixture.create(sdkVersion = Build.VERSION_CODES.S, executorService = executor) + val connectionStatusProvider = fixture.createProvider() + executor.runAll() + + connectionStatusProvider.close() + connectionStatusProvider.addConnectionStatusObserver(mock()) + executor.runAll() + + verify(fixture.telephonyManager, times(1)).registerTelephonyCallback(any(), any()) + } + + @Test + fun `foregrounding after close does not register the network callback again`() { + val executor = DeferredExecutorService() + val fixture = Fixture.create(executorService = executor) + val connectionStatusProvider = fixture.createProvider() + executor.runAll() + + connectionStatusProvider.close() + connectionStatusProvider.onForeground() + executor.runAll() + + assertThat(connectionStatusProvider.networkCallback).isNull() + verify(fixture.connectivityManager, times(1)).registerDefaultNetworkCallback(any()) + } + + @Test + fun `foregrounding after close does not notify child network callbacks of a lost network`() { + val executor = DeferredExecutorService() + val fixture = Fixture.create(executorService = executor) + val connectionStatusProvider = fixture.createProvider() + executor.runAll() + connectionStatusProvider.close() + executor.runAll() + val childNetworkCallback = mock() + fixture.addChildNetworkCallback(childNetworkCallback) + whenever(fixture.connectivityManager.activeNetwork).thenReturn(null) + + connectionStatusProvider.onForeground() + executor.runAll() + + verify(childNetworkCallback, never()).onLost(anyOrNull()) + } + /** Blocks a mocked system service call until released, or until a timeout elapses. */ private class Stall { private val entered = CountDownLatch(1) From 31aba53070be1aae3bd8f91f1352e6ea375eb206 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Tue, 10 Nov 2026 10:52:19 +0100 Subject: [PATCH 04/10] ref(android): Rename isConnected to onAvailableForwarded The flag doesn't track connectivity. It only stops child network callbacks from getting onAvailable again when the network callback is registered again on a return to the foreground. Name it for that and document it. Refs #6509 --- .../util/AndroidConnectionStatusProvider.java | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) 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 4d458cbef47..df35e115e9a 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 @@ -104,7 +104,14 @@ public final class AndroidConnectionStatusProvider private final int[] capabilityIds; private static final long CACHE_TTL_MINUTES = 2; - private final @NotNull AtomicBoolean isConnected = new AtomicBoolean(false); + + /** + * Flag for ensuring {@link #childCallbacks child callbacks} don't get more than one {@link + * NetworkCallback#onAvailable} invocation per connection. + * + *

Reset when the network is lost or unavailable. + */ + private final @NotNull AtomicBoolean onAvailableForwarded = new AtomicBoolean(false); public AndroidConnectionStatusProvider( @NotNull Context context, @@ -261,10 +268,9 @@ public void onAvailable(final @NotNull Network network) { current = snapshot.get(); } while (!snapshot.compareAndSet(current, current.withNetwork(network))); - // have to only dispatch this on first registration + when the connection got - // re-established - // otherwise it would've been dispatched on every foreground - if (!isConnected.getAndSet(true)) { + // Only forward the call on first registration + when the connection is + // re-established – not on every foreground. + if (!onAvailableForwarded.getAndSet(true)) { for (final @NotNull NetworkCallback cb : childCallbacks) { cb.onAvailable(network); } @@ -294,7 +300,7 @@ public void onLost(final @NotNull Network network) { } private void clearCacheAndNotifyObservers() { - isConnected.set(false); + onAvailableForwarded.set(false); snapshot.set(ConnectionSnapshot.lost(ticker)); options @@ -573,7 +579,7 @@ public void onForeground() { if (status == ConnectionStatus.DISCONNECTED) { // onLost is not called retroactively when we registerNetworkCallback (as opposed to // onAvailable), so we have to do it manually for the DISCONNECTED case - isConnected.set(false); + onAvailableForwarded.set(false); for (final @NotNull NetworkCallback cb : childCallbacks) { //noinspection DataFlowIssue cb.onLost(null); From 1f56d967823b932e8e2a3a8d5e7bd1316920372c Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Tue, 10 Nov 2026 10:52:36 +0100 Subject: [PATCH 05/10] fix(android): Build the connection cache debug log only when debug logging is on Without network capabilities, building the "Cache updated" message queried the ConnectivityManager up to twice more on the caller's thread, even with debug logging off. Refs #6509 --- .../util/AndroidConnectionStatusProvider.java | 14 ++++++---- .../AndroidConnectionStatusProviderTest.kt | 26 +++++++++++++++++++ 2 files changed, 35 insertions(+), 5 deletions(-) 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 df35e115e9a..b738926c98a 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 @@ -444,11 +444,15 @@ private boolean hasSignificantTransportChanges( return snapshot.get(); } - options - .getLogger() - .log( - SentryLevel.DEBUG, - "Cache updated - Status: " + statusOf(refreshed) + ", Type: " + typeOf(refreshed)); + // Without capabilities, statusOf and typeOf query the ConnectivityManager, so we only build the + // message if it'll be logged. + if (options.getLogger().isEnabled(SentryLevel.DEBUG)) { + options + .getLogger() + .log( + SentryLevel.DEBUG, + "Cache updated - Status: " + statusOf(refreshed) + ", Type: " + typeOf(refreshed)); + } return refreshed; } 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 d2e9c509ece..56cf8f4e0ba 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 @@ -22,6 +22,7 @@ import androidx.test.ext.junit.runners.AndroidJUnit4 import com.google.common.truth.Truth.assertThat import io.sentry.IConnectionStatusProvider import io.sentry.ILogger +import io.sentry.SentryLevel import io.sentry.SentryOptions import io.sentry.android.core.AppState import io.sentry.android.core.BuildInfoProvider @@ -418,6 +419,31 @@ class AndroidConnectionStatusProviderTest { provider.close() } + @Test + fun `a stale-cache refresh without network capabilities does not query the connectivity manager for its debug log when debug logging is off`() { + options.isDebug = false + whenever(networkInfo.isConnected).thenReturn(true) + whenever(connectivityManager.activeNetwork).thenReturn(null) + clearInvocations(connectivityManager) + + val status = connectionStatusProvider.connectionStatus + + assertThat(status).isEqualTo(IConnectionStatusProvider.ConnectionStatus.CONNECTED) + // Only the refresh and the returned status query; the debug log message is not built. + verify(connectivityManager, times(1)).activeNetwork + verify(connectivityManager, times(1)).activeNetworkInfo + } + + @Test + fun `a stale-cache refresh logs the refreshed connection status and type when debug logging is on`() { + options.isDebug = true + whenever(networkInfo.isConnected).thenReturn(true) + + connectionStatusProvider.connectionStatus + + verify(logger).log(eq(SentryLevel.DEBUG), eq("Cache updated - Status: CONNECTED, Type: wifi")) + } + @Test fun `cache TTL works correctly`() { // Setup: Mock network info to return connected From f804ac1e0096a320c340c89eee86a3d5eadea87c Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Tue, 10 Nov 2026 10:52:48 +0100 Subject: [PATCH 06/10] docs(android): Document AndroidConnectionStatusProvider's threading and exception policies Describe how clients use the provider, which calls may block, and the rules that keep the main thread from waiting: never lock the connection cache, never hold a lock while notifying, and only take the registration lock on the SDK executor. Also document the fields and group them, and make the snapshot's fields private. No behavior change. Refs #6509 --- .../util/AndroidConnectionStatusProvider.java | 240 +++++++++++------- 1 file changed, 150 insertions(+), 90 deletions(-) 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 b738926c98a..d4f153eff8f 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 @@ -9,6 +9,7 @@ import android.net.NetworkCapabilities; import android.os.Build; import android.os.Handler; +import android.telephony.TelephonyManager; import androidx.annotation.NonNull; import androidx.annotation.RequiresApi; import io.sentry.IConnectionStatusProvider; @@ -33,9 +34,51 @@ import org.jetbrains.annotations.TestOnly; /** - * Note: ConnectivityManager sometimes throws SecurityExceptions on Android 11. Hence all relevant - * calls are guarded with try/catch. see https://issuetracker.google.com/issues/175055271 for more - * details + * Primary source of network connectivity info for the Android SDK. Reports the status and type of + * the app's default + * network, plus cellular network generation when available. + * + *

Clients can either: + * + *

    + *
  • register callbacks to be notified of network updates as they occur (see {@link + * #addNetworkCallback} and {@link #addConnectionStatusObserver}), or + *
  • request network info synchronously (see, e.g., {@link #getConnection}). + *
+ * + * Note: Synchronous reads may make system calls on the calling thread if they're not backed by + * cached data or if their cached data has gone stale. + * + *

Exception policy + * + *

The provider sources its network info from {@link ConnectivityManager}, which sometimes throws + * SecurityExceptions on Android 11. Hence all relevant calls are guarded with try/catch. See here for more details. + * + *

Threading policy + * + *

Public methods may be called from any thread. Client registrations are added and removed + * synchronously using thread-safe lists; they do not wait for this provider's own callbacks + * to be registered with {@link ConnectivityManager} or {@link TelephonyManager}. Registration and + * unregistration of those system callbacks run asynchronously on the {@link + * SentryOptions#getExecutorService() SDK executor}. + * + *

The main thread queries this provider during {@code SentryAndroid.init} and whenever an event + * is captured. To avoid blocking the main thread, be sure to: + * + *

    + *
  • Never lock the connection cache. Instead, replace the whole {@link ConnectionSnapshot} via + * a compare-and-set operation, and read it once per operation. + *
  • Never hold a lock while notifying observers or child network callbacks. Both run SDK code + * that could invoke user callbacks such as {@code beforeBreadcrumb}, and those callbacks can + * block or deadlock. + *
  • Only acquire {@link #registrationLock} on the SDK executor. It's held across network + * callback registration, which can be long-lasting. + *
  • Check from the SDK executor whether the callbacks are registered, not from the calling + * thread. The executor runs tasks in submission order, so it's guaranteed to see the latest + * registration state. + *
*/ @ApiStatus.Internal public final class AndroidConnectionStatusProvider @@ -47,47 +90,41 @@ public final class AndroidConnectionStatusProvider private final @NotNull MonotonicTicker ticker; /** - * Copy-on-write, so that observers can be added and removed from any thread without waiting on a - * lock, including from inside a notification. Each notification goes to the observers present - * when it started. + * Registered client observers. Provider notifies them about the network's {@link + * ConnectionStatus}. + * + *

Used by SDK integrations, including Session Replay and integrations that send cached + * envelopes. */ - private final @NotNull List connectionStatusObservers; + private final @NotNull List connectionStatusObservers = + new CopyOnWriteArrayList<>(); private final @NotNull CellularNetworkTechnologyProvider cellularNetworkTechnologyProvider; private final @Nullable Handler handler; /** - * What the provider currently knows about the default network. Replaced as a whole, never - * modified, so readers on any thread, including the main thread when it captures an event, see a - * consistent value without taking a lock. - */ - private final @NotNull AtomicReference snapshot; - - /** - * Serializes registering and unregistering the network and telephony callbacks. These are - * synchronous calls into system services that can take a long time, so the lock is only taken on - * the SDK executor, and nothing that callers wait for is done while it is held. + * Serializes registration and unregistration of the callbacks this provider's passes to {@link + * ConnectivityManager} and {@link TelephonyManager}. + * + *

Note: The callbacks registered with ConnectivityManager and TelephonyManager, which are + * guarded by this lock, are distinct from the {@link #connectionStatusObservers} and {@link + * #childCallbacks} registered by this provider's own clients. (The latter are made thread-safe + * via {@link CopyOnWriteArrayList}s.) */ private final @NotNull AutoClosableReentrantLock registrationLock = new AutoClosableReentrantLock(); private volatile @Nullable NetworkCallback networkCallback; - /** - * Set by {@link #close()} on the calling thread, before the unregistration it schedules runs. - * Registration and foreground refreshes queued on the SDK executor after that unregistration - * check it, so they don't register the callbacks again on a closed provider. - */ - private volatile boolean closed; - private static final @NotNull AutoClosableReentrantLock connectivityManagerLock = new AutoClosableReentrantLock(); private static volatile @Nullable ConnectivityManager connectivityManager; /** - * Copy-on-write, so that callbacks are dispatched from a snapshot without holding a lock. The - * callbacks run SDK code such as adding breadcrumbs, which in turn runs user callbacks, while - * other threads, including the main thread, add and remove callbacks. + * Registered client callbacks. Provider invokes them to forward network connectivity events + * emitted by {@link ConnectivityManager}. + * + *

Used by SDK integrations, including network breadcrumbs. */ private static final @NotNull List childCallbacks = new CopyOnWriteArrayList<>(); @@ -99,7 +136,10 @@ public final class AndroidConnectionStatusProvider }; /** - * The capabilities whose change affects the connection status, which depends on the API level. + * IDs of {@link NetworkCapabilities} whose status can affect whether a network is considered + * connected. + * + *

The relevant capabilities vary by Android API level. */ private final int[] capabilityIds; @@ -113,6 +153,27 @@ public final class AndroidConnectionStatusProvider */ private final @NotNull AtomicBoolean onAvailableForwarded = new AtomicBoolean(false); + /** + * Cache of the latest network connection info. + * + *

Threading policy + * + *

This container and the snapshots it wraps are shared across threads. Updates publish a new + * snapshot via the atomic reference. Synchronous requests for network info are expected to use + * compare-and-set to discard the system's response if another update replaces the snapshot while + * the synchronous request runs. + */ + private final @NotNull AtomicReference snapshot; + + /** + * True if this provider has been {@link #close closed}, and therefore must permanently stop + * monitoring network connectivity. + * + *

If true, further callback registration and foreground refreshes are skipped. Existing + * observers and callbacks may be awaiting cleanup, as cleanup runs asynchronously. + */ + private volatile boolean closed; + public AndroidConnectionStatusProvider( @NotNull Context context, @NotNull SentryOptions options, @@ -134,7 +195,6 @@ public AndroidConnectionStatusProvider( this.ticker = ticker; this.snapshot = new AtomicReference<>(ConnectionSnapshot.unknown(ticker)); this.handler = handler; - this.connectionStatusObservers = new CopyOnWriteArrayList<>(); this.cellularNetworkTechnologyProvider = new CellularNetworkTechnologyProvider(this.context, options.getLogger(), buildInfoProvider); @@ -188,8 +248,8 @@ private boolean isNetworkEffectivelyConnected( } /** - * The connection status of a snapshot, or of the legacy {@link ConnectivityManager} query when - * the snapshot has no capabilities. + * Returns the connection status of a snapshot, or of the legacy {@link ConnectivityManager} query + * if the snapshot has no {@link ConnectionSnapshot#capabilities capabilities}. */ private @NotNull ConnectionStatus statusOf(final @NotNull ConnectionSnapshot snapshot) { final @Nullable NetworkCapabilities capabilities = snapshot.capabilities; @@ -210,8 +270,10 @@ private boolean isNetworkEffectivelyConnected( } /** - * The connection type of a snapshot, or of the legacy {@link ConnectivityManager} query when the - * snapshot has no capabilities. + * Returns the {@link #getConnectionType(NetworkCapabilities) connection type} of a snapshot, or + * of the {@link #getConnectionType(Context, ILogger, BuildInfoProvider) legacy + * ConnectivityManager query} if the snapshot has no {@link ConnectionSnapshot#capabilities + * capabilities}. */ private @Nullable String typeOf(final @NotNull ConnectionSnapshot snapshot) { final @Nullable NetworkCapabilities capabilities = snapshot.capabilities; @@ -225,9 +287,6 @@ private boolean isNetworkEffectivelyConnected( /** * The connection type and, for cellular connections, the generation of the network technology. - * - *

The type is derived from a single snapshot, so it can't mix two networks across a - * connectivity change. */ public @NotNull NetworkConnection getConnection() { final @Nullable String connectionType = typeOf(freshSnapshot()); @@ -253,8 +312,8 @@ private void ensureNetworkCallbackRegistered() { final boolean registered; try (final @NotNull ISentryLifecycleToken ignored = registrationLock.acquire()) { - // Checked again under the lock in case close() ran in the meantime: its unregistration takes - // this lock too, so either it sees the registration and undoes it, or this sees the flag. + // Registration and unregistration share this lock. Rechecking the closed flag here ensures + // that closing the provider prevents registration and cleans up the registered callbacks. if (closed || networkCallback != null) { return; } @@ -328,8 +387,10 @@ public void onCapabilitiesChanged( } /** - * Stores the capabilities and notifies observers, but only if they differ from the - * stored ones in a way that changes the connection status or type. + * Caches the capabilities and notifies observers. + * + *

No-ops unless the incoming {@code networkCapabilities} differ from the stored ones + * in a way relevant to the connection status or type. */ private void updateCacheAndNotifyObservers( final @NotNull NetworkCapabilities networkCapabilities) { @@ -421,19 +482,19 @@ private boolean hasSignificantTransportChanges( } } - /** The current snapshot, refreshed first if it is stale. */ + /** The current snapshot, refreshed first if it's stale. */ private @NotNull ConnectionSnapshot freshSnapshot() { final @NotNull ConnectionSnapshot current = snapshot.get(); return current.isFresh() ? current : refresh(current); } /** - * Queries the active network and stores its capabilities, unless the snapshot changed during the - * query. + * Queries the active network and stores its {@link NetworkCapabilities capabilities}, unless the + * snapshot changed during the query. * - * @param before the snapshot the refresh replaces + * @param before the snapshot the refresh request replaces * @return the stored snapshot, or, if another thread such as a network callback replaced {@code - * before} during the query, that newer snapshot, which may still be stale + * before} during the query, that newer snapshot */ private @NotNull ConnectionSnapshot refresh(final @NotNull ConnectionSnapshot before) { final @NotNull ConnectionSnapshot refreshed = @@ -459,8 +520,8 @@ private boolean hasSignificantTransportChanges( /** * Queries the capabilities of the active network from the {@link ConnectivityManager}. * - * @return the capabilities, or {@code null} if they can't be read: without permission, below API - * 23, without an active network, or when the query fails + * @return the capabilities, or {@code null} if they can't be read (i.e., below API 23, without + * permission, without an active network, or when the query fails) */ @SuppressLint({"NewApi", "MissingPermission"}) private @Nullable NetworkCapabilities queryActiveNetworkCapabilities() { @@ -502,11 +563,12 @@ private boolean hasSignificantTransportChanges( } /** - * Adds an observer and makes sure the network callback is registered. + * Adds an observer synchronously and makes sure the network callback is registered + * asynchronously. * *

The observer is added immediately. Registering the network callback calls into system - * services that can be slow, so it is scheduled on the SDK executor instead of making the caller, - * often the main thread during {@code Sentry.init}, wait for it. + * services that can be slow, so it's scheduled on the SDK executor instead of making the caller + * wait for it. (The caller is often the main thread during {@code Sentry.init}.) * * @return always {@code true}, because the observer is added even if registering the network * callback is still pending or fails @@ -514,7 +576,7 @@ private boolean hasSignificantTransportChanges( @Override public boolean addConnectionStatusObserver(final @NotNull IConnectionStatusObserver observer) { connectionStatusObservers.add(observer); - // The executor drops tasks when its queue is full, which would skip this registration. That is + // The executor drops tasks when its queue is full, which would skip this registration. That's // unlikely, and onForeground() registers the callback again the next time the app is shown. //noinspection Convert2MethodRef submitSafe(() -> ensureNetworkCallbackRegistered()); @@ -549,6 +611,7 @@ private void unregisterNetworkCallback(final boolean clearObservers) { @Override public void close() { closed = true; + submitSafe( () -> { unregisterNetworkCallback(/* clearObservers= */ true); @@ -561,18 +624,16 @@ public void close() { } /** - * Refreshes the cache, notifies observers and registers the network callback, unless it is - * already registered or the provider is closed. - * - *

Whether the callback is registered is checked on the SDK executor, which also runs - * registration and unregistration in submission order. Checking on the calling thread instead - * would skip this work when the app returns to the foreground before the unregistration queued by - * {@link #onBackground()} has run, leaving the callback unregistered while in the foreground. + * Schedules a task to refresh the cache, notify observers and register the network callback. The + * task does nothing if the callback is already registered or the provider is closed. */ @Override public void onForeground() { submitSafe( () -> { + // Check on the executor so any earlier background unregistration runs before we decide + // whether registration is needed. Checking before submission could leave the callback + // unregistered after a quick return to the foreground. if (closed || networkCallback != null) { return; } @@ -595,25 +656,22 @@ public void onForeground() { // this will ONLY do the necessary parts like registerNetworkCallback and onAvailable, but // it won't refresh the cache and notify observers because we just did it above and the - // cached - // capabilities will be the same + // cached capabilities will be the same ensureNetworkCallbackRegistered(); }); } /** - * Unregisters the network callback and the cellular technology listener, if the network callback - * is registered. - * - *

Whether the callback is registered is checked on the SDK executor, which also runs - * registration and unregistration in submission order. Checking on the calling thread instead - * would skip unregistering when the app goes to the background while a registration is queued or - * running, leaving the callbacks registered in the background. + * Schedules unregistration of the network callback and the cellular technology listener, if + * registered. */ @Override public void onBackground() { submitSafe( () -> { + // Check on the executor so any earlier registration finishes before we decide whether + // to unregister. Checking before submission could miss a pending registration and leave + // the callbacks registered in the background. if (networkCallback == null) { return; } @@ -622,8 +680,8 @@ public void onBackground() { } /** - * Get the cached NetworkCapabilities for advanced use cases. Returns null if cache is stale or no - * capabilities are available. + * Get the cached NetworkCapabilities for advanced use cases. Returns whatever the current + * snapshot holds, even if it is stale, or null if it has no capabilities. * * @return cached NetworkCapabilities or null */ @@ -907,24 +965,27 @@ public static List getChildCallbacks() { } /** - * An immutable view of the default network: which network it is, its capabilities, and until when - * they can be used without querying the {@link ConnectivityManager} again. + * An atomically publishable snapshot of cached network connection info, obtained from {@link + * ConnectivityManager}. * - *

Each change replaces the whole snapshot, so a reader that reads it once gets values that - * belong together, and a refresh can detect with a compare-and-set that a network callback - * replaced it during the query. + *

Note: This class's {@link #network} and {@link #capabilities} fields are updated + * independently and can describe different networks during handover (e.g., from cellular to + * Wi-Fi). That behavior predates this class and - for now – isn't problematic, as no caller + * requires the two fields to describe the same network. (Current readers use {@code network} to + * filter callbacks and {@code capabilities} to derive connection info.) */ private static final class ConnectionSnapshot { - /** The default network reported by the network callback, or {@code null} if none is known. */ - final @Nullable Network network; + + /** Info about the network of interest, or {@code null} if nothing is known. */ + private final @Nullable Network network; /** - * The capabilities of the default network, or {@code null} if they are unknown or there is no - * default network. Readers then fall back to the legacy {@link ConnectivityManager} query. + * The capabilities of the network. {@code null} if they're unknown or there's no known network. */ - final @Nullable NetworkCapabilities capabilities; + private final @Nullable NetworkCapabilities capabilities; - final @NotNull Deadline freshUntil; + /** When this snapshot becomes stale and {@link ConnectivityManager} should be requeried. */ + private final @NotNull Deadline freshUntil; private ConnectionSnapshot( final @Nullable Network network, @@ -935,30 +996,29 @@ private ConnectionSnapshot( this.freshUntil = freshUntil; } - /** Nothing is known, so the next read queries the active network. */ + /** Returns {@code true} if this snapshot has yet to go stale. */ + boolean isFresh() { + return !freshUntil.hasPassed(); + } + + /** Indicates that nothing is known about the network. */ static @NotNull ConnectionSnapshot unknown(final @NotNull MonotonicTicker ticker) { return new ConnectionSnapshot(null, null, Deadline.passed(ticker)); } - /** - * The network callback reported that the default network is gone. The snapshot is not refreshed - * until the cache time-to-live has elapsed, but it has no capabilities, so reading the status - * or type still falls back to the legacy {@link ConnectivityManager} query on every read. - */ + /** Indicates that the network has been lost. */ static @NotNull ConnectionSnapshot lost(final @NotNull MonotonicTicker ticker) { return new ConnectionSnapshot( null, null, Deadline.after(ticker, CACHE_TTL_MINUTES, TimeUnit.MINUTES)); } - boolean isFresh() { - return !freshUntil.hasPassed(); - } - + /** Returns a copy of this snapshot with the provided {@code network}. */ @NotNull ConnectionSnapshot withNetwork(final @NotNull Network network) { return new ConnectionSnapshot(network, capabilities, freshUntil); } + /** Returns a copy of this snapshot with the provided {@code capabilities} and deadline. */ @NotNull ConnectionSnapshot withCapabilities( final @Nullable NetworkCapabilities capabilities, final @NotNull Deadline freshUntil) { From f2a7096490b3754803bd9bf0f1e39e27f59fb80a Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Tue, 10 Nov 2026 10:52:56 +0100 Subject: [PATCH 07/10] test(android): Tighten connection status provider concurrency tests Make tests that passed on the pre-fix code fail on it: let close() run before the follow-up call, and keep removeConnectionStatusObserver from waiting in its own setup. Check what a reader racing a network callback gets back, and fail tests whose background notification threads throw or hang. Add tests that getConnectionType, adding or removing an observer during a notification, and reading a fresh cache during another caller's refresh don't wait. Drop a test that pinned the cached capabilities instance, remove a duplicate static-state reset, and shorten names. Refs #6509 --- ...ConnectionStatusProviderConcurrencyTest.kt | 170 ++++++++++++------ 1 file changed, 113 insertions(+), 57 deletions(-) diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderConcurrencyTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderConcurrencyTest.kt index 80d34d06a11..86959ae7b15 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderConcurrencyTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderConcurrencyTest.kt @@ -16,6 +16,7 @@ import android.telephony.TelephonyCallback import android.telephony.TelephonyManager import androidx.test.ext.junit.runners.AndroidJUnit4 import com.google.common.truth.Truth.assertThat +import com.google.common.truth.Truth.assertWithMessage import io.sentry.IConnectionStatusProvider.ConnectionStatus import io.sentry.IConnectionStatusProvider.IConnectionStatusObserver import io.sentry.ILogger @@ -25,7 +26,6 @@ import io.sentry.android.core.AppState import io.sentry.android.core.BuildInfoProvider import io.sentry.android.core.ContextUtils import io.sentry.test.DeferredExecutorService -import io.sentry.test.ImmediateExecutorService import io.sentry.time.TestMonotonicTicker import java.util.concurrent.Callable import java.util.concurrent.CountDownLatch @@ -87,7 +87,7 @@ class AndroidConnectionStatusProviderConcurrencyTest { } @Test - fun `getConnectionStatus refreshes a stale cache without waiting for a stalled network callback registration`() { + fun `getConnectionStatus refreshes an empty cache without waiting for a stalled network callback registration`() { val fixture = Fixture.create() val stall = fixture.stallNetworkCallbackRegistration() val connectionStatusProvider = fixture.createProvider() @@ -99,7 +99,19 @@ class AndroidConnectionStatusProviderConcurrencyTest { } @Test - fun `getConnection refreshes a stale cache without waiting for a stalled network callback registration`() { + fun `getConnectionType refreshes an empty cache without waiting for a stalled network callback registration`() { + val fixture = Fixture.create() + val stall = fixture.stallNetworkCallbackRegistration() + val connectionStatusProvider = fixture.createProvider() + stall.awaitEntered() + + val type = fixture.onCallerThread { connectionStatusProvider.connectionType } + + assertThat(type).isEqualTo("wifi") + } + + @Test + fun `getConnection refreshes an empty cache without waiting for a stalled network callback registration`() { val fixture = Fixture.create() val stall = fixture.stallNetworkCallbackRegistration() val connectionStatusProvider = fixture.createProvider() @@ -131,11 +143,14 @@ class AndroidConnectionStatusProviderConcurrencyTest { @Test fun `removeConnectionStatusObserver does not wait for a stalled telephony callback registration`() { val fixture = Fixture.create(sdkVersion = Build.VERSION_CODES.S) - val stall = fixture.stallTelephonyCallbackRegistration() - val connectionStatusProvider = fixture.createProvider() - stall.awaitEntered() + val connectionStatusProvider = fixture.createRegisteredProvider() val observer = mock() connectionStatusProvider.addConnectionStatusObserver(observer) + connectionStatusProvider.onBackground() + fixture.awaitExecutorIdle() + val stall = fixture.stallTelephonyCallbackRegistration() + connectionStatusProvider.onForeground() + stall.awaitEntered() fixture.onCallerThread { connectionStatusProvider.removeConnectionStatusObserver(observer) } @@ -143,7 +158,7 @@ class AndroidConnectionStatusProviderConcurrencyTest { } @Test - fun `getConnectionStatus refreshes a stale cache without waiting for a stalled telephony callback registration`() { + fun `getConnectionStatus refreshes an empty cache without waiting for a stalled telephony callback registration`() { val fixture = Fixture.create(sdkVersion = Build.VERSION_CODES.S) val stall = fixture.stallTelephonyCallbackRegistration() val connectionStatusProvider = fixture.createProvider() @@ -174,7 +189,7 @@ class AndroidConnectionStatusProviderConcurrencyTest { } @Test - fun `getConnectionStatus refreshes a stale cache without waiting for a stalled network callback unregistration`() { + fun `getConnectionStatus refreshes an empty cache without waiting for a stalled network callback unregistration`() { val fixture = Fixture.create() val connectionStatusProvider = fixture.createRegisteredProvider() val stall = fixture.stallNetworkCallbackUnregistration() @@ -204,7 +219,7 @@ class AndroidConnectionStatusProviderConcurrencyTest { } @Test - fun `getConnectionStatus refreshes a stale cache without waiting for a stalled telephony callback unregistration`() { + fun `getConnectionStatus refreshes an empty cache without waiting for a stalled telephony callback unregistration`() { val fixture = Fixture.create(sdkVersion = Build.VERSION_CODES.S) val connectionStatusProvider = fixture.createRegisteredProvider() val stall = fixture.stallTelephonyCallbackUnregistration() @@ -216,40 +231,29 @@ class AndroidConnectionStatusProviderConcurrencyTest { assertThat(status).isEqualTo(ConnectionStatus.CONNECTED) } - // A stale-cache refresh racing a network callback + // An in-flight cache refresh racing a network callback @Test - fun `a stale-cache refresh stores the queried capabilities of the active network`() { - val fixture = Fixture.create() - val connectionStatusProvider = fixture.createRegisteredProvider() - - connectionStatusProvider.connectionStatus - - assertThat(connectionStatusProvider.cachedNetworkCapabilities) - .isSameInstanceAs(fixture.wifiCapabilities) - } - - @Test - fun `capabilities changed while a stale-cache refresh queries the network capabilities are kept`() { + fun `a network capabilities change survives an in-flight cache refresh`() { val fixture = Fixture.create() val connectionStatusProvider = fixture.createRegisteredProvider() val networkCallback = fixture.captureNetworkCallback() networkCallback.onAvailable(fixture.network) val stall = fixture.stallNetworkCapabilitiesQuery() - val refresh = fixture.startOnCallerThread { connectionStatusProvider.connectionStatus } + val refresh = fixture.startOnCallerThread { connectionStatusProvider.connectionType } stall.awaitEntered() fixture.onCallerThread { networkCallback.onCapabilitiesChanged(fixture.network, fixture.cellularCapabilities) } stall.release() - refresh.get(TIMEOUT_SECONDS, SECONDS) + val type = refresh.get(TIMEOUT_SECONDS, SECONDS) - assertThat(connectionStatusProvider.connectionType).isEqualTo("cellular") + assertThat(type).isEqualTo("cellular") } @Test - fun `a network lost while a stale-cache refresh queries the network capabilities keeps the cache cleared`() { + fun `a network loss survives an in-flight cache refresh`() { val fixture = Fixture.create() val connectionStatusProvider = fixture.createRegisteredProvider() val networkCallback = fixture.captureNetworkCallback() @@ -266,7 +270,7 @@ class AndroidConnectionStatusProviderConcurrencyTest { } @Test - fun `a network that becomes available while a stale-cache refresh queries the network capabilities stays the current network`() { + fun `a network that becomes available during an in-flight cache refresh stays the current network`() { val fixture = Fixture.create() val connectionStatusProvider = fixture.createRegisteredProvider() val networkCallback = fixture.captureNetworkCallback() @@ -284,10 +288,72 @@ class AndroidConnectionStatusProviderConcurrencyTest { verify(observer).onConnectionStatusChanged(ConnectionStatus.DISCONNECTED) } - // Observers changing the observers during a notification + @Test + fun `getConnectionType reads a fresh cache without waiting for another caller's in-flight cache refresh`() { + val fixture = Fixture.create() + val connectionStatusProvider = fixture.createRegisteredProvider() + val networkCallback = fixture.captureNetworkCallback() + networkCallback.onAvailable(fixture.network) + val stall = fixture.stallNetworkCapabilitiesQuery() + val refresh = fixture.startOnCallerThread { connectionStatusProvider.connectionType } + stall.awaitEntered() + fixture.onCallerThread { + networkCallback.onCapabilitiesChanged(fixture.network, fixture.cellularCapabilities) + } + + val type = fixture.onCallerThread { connectionStatusProvider.connectionType } + stall.release() + refresh.get(TIMEOUT_SECONDS, SECONDS) + + assertThat(type).isEqualTo("cellular") + } + + // Observers being notified + + @Test + fun `addConnectionStatusObserver does not wait for an observer that is being notified`() { + val fixture = Fixture.create() + val connectionStatusProvider = fixture.createRegisteredProvider() + val networkCallback = fixture.captureNetworkCallback() + networkCallback.onAvailable(fixture.network) + val stall = fixture.newStall() + connectionStatusProvider.addConnectionStatusObserver { stall.block(Unit) } + val notification = fixture.startOnCallerThread { networkCallback.onLost(fixture.network) } + stall.awaitEntered() + val observer = mock() + + val added = fixture.onCallerThread { + connectionStatusProvider.addConnectionStatusObserver(observer) + } + stall.release() + notification.get(TIMEOUT_SECONDS, SECONDS) + + assertThat(added).isTrue() + assertThat(connectionStatusProvider.statusObservers).contains(observer) + } + + @Test + fun `removeConnectionStatusObserver does not wait for an observer that is being notified`() { + val fixture = Fixture.create() + val connectionStatusProvider = fixture.createRegisteredProvider() + val networkCallback = fixture.captureNetworkCallback() + networkCallback.onAvailable(fixture.network) + val stall = fixture.newStall() + connectionStatusProvider.addConnectionStatusObserver { stall.block(Unit) } + val observer = mock() + connectionStatusProvider.addConnectionStatusObserver(observer) + val notification = fixture.startOnCallerThread { networkCallback.onLost(fixture.network) } + stall.awaitEntered() + + fixture.onCallerThread { connectionStatusProvider.removeConnectionStatusObserver(observer) } + stall.release() + notification.get(TIMEOUT_SECONDS, SECONDS) + + assertThat(connectionStatusProvider.statusObservers).doesNotContain(observer) + } @Test - fun `an observer that removes itself during a notification does not keep the other observers from being notified`() { + fun `an observer removing itself during a notification does not stop delivery to the other observers`() { val fixture = Fixture.create() val connectionStatusProvider = fixture.createRegisteredProvider() val networkCallback = fixture.captureNetworkCallback() @@ -348,7 +414,7 @@ class AndroidConnectionStatusProviderConcurrencyTest { } } ) - fixture.startOnCallerThread { networkCallback.onAvailable(fixture.network) } + val notification = fixture.startOnCallerThread { networkCallback.onAvailable(fixture.network) } stall.awaitEntered() val childNetworkCallback = mock() @@ -360,6 +426,8 @@ class AndroidConnectionStatusProviderConcurrencyTest { childNetworkCallback, ) } + stall.release() + notification.get(TIMEOUT_SECONDS, SECONDS) assertThat(added).isTrue() assertThat(AndroidConnectionStatusProvider.getChildCallbacks()).contains(childNetworkCallback) @@ -380,19 +448,21 @@ class AndroidConnectionStatusProviderConcurrencyTest { ) val childNetworkCallback = mock() fixture.addChildNetworkCallback(childNetworkCallback) - fixture.startOnCallerThread { networkCallback.onAvailable(fixture.network) } + val notification = fixture.startOnCallerThread { networkCallback.onAvailable(fixture.network) } stall.awaitEntered() fixture.onCallerThread { AndroidConnectionStatusProvider.removeNetworkCallback(childNetworkCallback) } + stall.release() + notification.get(TIMEOUT_SECONDS, SECONDS) assertThat(AndroidConnectionStatusProvider.getChildCallbacks()) .doesNotContain(childNetworkCallback) } @Test - fun `a child network callback that removes itself during a notification does not keep the other child network callbacks from being notified`() { + fun `a child network callback removing itself during a notification does not stop delivery to the other child network callbacks`() { val fixture = Fixture.create() fixture.createRegisteredProvider() val networkCallback = fixture.captureNetworkCallback() @@ -520,8 +590,9 @@ class AndroidConnectionStatusProviderConcurrencyTest { val fixture = Fixture.create(executorService = executor) val connectionStatusProvider = fixture.createProvider() executor.runAll() - connectionStatusProvider.close() + executor.runAll() + connectionStatusProvider.addConnectionStatusObserver(mock()) executor.runAll() @@ -535,8 +606,9 @@ class AndroidConnectionStatusProviderConcurrencyTest { val fixture = Fixture.create(sdkVersion = Build.VERSION_CODES.S, executorService = executor) val connectionStatusProvider = fixture.createProvider() executor.runAll() - connectionStatusProvider.close() + executor.runAll() + connectionStatusProvider.addConnectionStatusObserver(mock()) executor.runAll() @@ -549,8 +621,9 @@ class AndroidConnectionStatusProviderConcurrencyTest { val fixture = Fixture.create(executorService = executor) val connectionStatusProvider = fixture.createProvider() executor.runAll() - connectionStatusProvider.close() + executor.runAll() + connectionStatusProvider.onForeground() executor.runAll() @@ -764,6 +837,7 @@ class AndroidConnectionStatusProviderConcurrencyTest { // A caller that is still running after close() would cache this test's connectivity // manager in the static field that close() resets, and leak it into the next test. callerThreads.forEach { it.join(SECONDS.toMillis(TIMEOUT_SECONDS)) } + val hungCallerThreads = callerThreads.count { it.isAlive } connectionStatusProvider?.close() when (executorService) { is SingleThreadExecutorService -> executorService.shutdown() @@ -771,6 +845,9 @@ class AndroidConnectionStatusProviderConcurrencyTest { } contextUtils.close() AppState.getInstance().unregisterLifecycleObserver() + assertWithMessage("Caller threads still running after their stalls were released") + .that(hungCallerThreads) + .isEqualTo(0) } companion object { @@ -779,34 +856,13 @@ class AndroidConnectionStatusProviderConcurrencyTest { fun create( sdkVersion: Int = Build.VERSION_CODES.N, executorService: ISentryExecutorService = SingleThreadExecutorService(), - ): Fixture { - resetStaticState() - return Fixture(sdkVersion, executorService).also { created += it } - } + ): Fixture = Fixture(sdkVersion, executorService).also { created += it } fun closeAll() { created.forEach { it.close() } created.clear() } - /** - * Clears the cached ConnectivityManager and the child network callbacks, which are static and - * may have been left behind by other suites, by closing a provider that registers nothing. - */ - private fun resetStaticState() { - val resetOptions = SentryOptions().apply { executorService = ImmediateExecutorService() } - val foregroundImportance = - mockStatic(ContextUtils::class.java, CALLS_REAL_METHODS).also { - it.`when` { ContextUtils.isForegroundImportance() }.thenReturn(false) - } - try { - AndroidConnectionStatusProvider(mock(), resetOptions, mock(), TestMonotonicTicker()) - .close() - } finally { - foregroundImportance.close() - } - } - private fun capabilities(transport: Int): NetworkCapabilities { val capabilities = mock() whenever(capabilities.hasCapability(NET_CAPABILITY_INTERNET)).thenReturn(true) From 9faedfbf90df4443582978131fb2991c6ffe8f02 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Tue, 10 Nov 2026 10:56:49 +0100 Subject: [PATCH 08/10] changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cde07fbb999..8680b4ceb62 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,7 +21,7 @@ ### Fixes -- Prevent the main thread from potentially hanging during app start if Android is slow to register the SDK's network or telephony listeners. `Sentry.init`, Session Replay, and capturing events no longer wait for that registration ([#XXXX](https://github.com/getsentry/sentry-java/issues/XXXX)) +- Prevent the main thread from potentially hanging during app start if Android is slow to register the SDK's network or telephony listeners. `Sentry.init`, Session Replay, and capturing events no longer wait for that registration ([#6537](https://github.com/getsentry/sentry-java/pull/6537)) - Add opt-in support for matching GraphQL ignored error types against classification specifications via `sentry.graphql.enable-specification-error-type-filtering` ([#6092](https://github.com/getsentry/sentry-java/pull/6092)) - Session Replay: Fix `SentryOkHttpInterceptor` hanging forever on responses whose body has no known length, such as Server-Sent Events or a gzipped response, when network details are enabled for the URL ([#6231](https://github.com/getsentry/sentry-java/pull/6231)) - Session Replay: `SentryOkHttpInterceptor` now captures network request and response details only while Session Replay is recording or buffering ([#6486](https://github.com/getsentry/sentry-java/pull/6486)) From 43d74a6280bd44a83febcbd02ee47d4c3300f5ca Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Tue, 10 Nov 2026 12:17:32 +0100 Subject: [PATCH 09/10] fix(android): ADDRESS PR COMMENT: Keep a cache refresh's result when the network changes during it onAvailable replaces the connection snapshot with one that only has the new network, keeping the old capabilities and expired deadline. A refresh racing it saw the snapshot change, returned that stale snapshot and discarded its own query, so a read during a network handover could report the previous network's type or status. A refresh now gives way only to a fresh snapshot. If the newer snapshot is stale, the refresh stores its result on it, keeping its network. Refs #6509 --- .../util/AndroidConnectionStatusProvider.java | 37 ++++++++++++------- ...ConnectionStatusProviderConcurrencyTest.kt | 31 ++++++++++++++-- 2 files changed, 52 insertions(+), 16 deletions(-) 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 d4f153eff8f..876c5fb48fb 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 @@ -159,9 +159,9 @@ public final class AndroidConnectionStatusProvider *

Threading policy * *

This container and the snapshots it wraps are shared across threads. Updates publish a new - * snapshot via the atomic reference. Synchronous requests for network info are expected to use - * compare-and-set to discard the system's response if another update replaces the snapshot while - * the synchronous request runs. + * snapshot via the atomic reference. Synchronous requests for network info store the system's + * response with compare-and-set, and discard it only if another update publishes a fresh snapshot + * while the request runs. */ private final @NotNull AtomicReference snapshot; @@ -489,20 +489,31 @@ private boolean hasSignificantTransportChanges( } /** - * Queries the active network and stores its {@link NetworkCapabilities capabilities}, unless the - * snapshot changed during the query. + * Queries the active network and stores its {@link NetworkCapabilities capabilities}. + * + *

If another thread, such as a network callback, replaces the snapshot during the query, the + * result is still stored as long as the newer snapshot is stale, keeping that snapshot's network. + * For example, {@code onAvailable} only swaps in the network during a network handover. A fresh + * newer snapshot has more recent capabilities, or records a lost network, so it's kept instead. * * @param before the snapshot the refresh request replaces - * @return the stored snapshot, or, if another thread such as a network callback replaced {@code - * before} during the query, that newer snapshot + * @return the stored snapshot, or the fresh snapshot another thread stored during the query */ private @NotNull ConnectionSnapshot refresh(final @NotNull ConnectionSnapshot before) { - final @NotNull ConnectionSnapshot refreshed = - before.withCapabilities( - queryActiveNetworkCapabilities(), - Deadline.after(ticker, CACHE_TTL_MINUTES, TimeUnit.MINUTES)); - if (!snapshot.compareAndSet(before, refreshed)) { - return snapshot.get(); + final @Nullable NetworkCapabilities capabilities = queryActiveNetworkCapabilities(); + final @NotNull Deadline freshUntil = + Deadline.after(ticker, CACHE_TTL_MINUTES, TimeUnit.MINUTES); + @NotNull ConnectionSnapshot current = before; + @NotNull ConnectionSnapshot refreshed; + while (true) { + refreshed = current.withCapabilities(capabilities, freshUntil); + if (snapshot.compareAndSet(current, refreshed)) { + break; + } + current = snapshot.get(); + if (current.isFresh()) { + return current; + } } // Without capabilities, statusOf and typeOf query the ConnectivityManager, so we only build the diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderConcurrencyTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderConcurrencyTest.kt index 86959ae7b15..d201d48d111 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderConcurrencyTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderConcurrencyTest.kt @@ -33,6 +33,7 @@ import java.util.concurrent.ExecutorService import java.util.concurrent.Executors import java.util.concurrent.Future import java.util.concurrent.FutureTask +import java.util.concurrent.TimeUnit.MINUTES import java.util.concurrent.TimeUnit.SECONDS import java.util.concurrent.TimeoutException import kotlin.test.AfterTest @@ -288,6 +289,25 @@ class AndroidConnectionStatusProviderConcurrencyTest { verify(observer).onConnectionStatusChanged(ConnectionStatus.DISCONNECTED) } + @Test + fun `an in-flight refresh of a stale cache keeps its query result when another network becomes available`() { + val fixture = Fixture.create() + val connectionStatusProvider = fixture.createRegisteredProvider() + val networkCallback = fixture.captureNetworkCallback() + networkCallback.onAvailable(fixture.network) + networkCallback.onCapabilitiesChanged(fixture.network, fixture.wifiCapabilities) + fixture.ticker.advance(CACHE_EXPIRY_MINUTES, MINUTES) + val stall = fixture.stallNetworkCapabilitiesQuery(result = fixture.cellularCapabilities) + val refresh = fixture.startOnCallerThread { connectionStatusProvider.connectionType } + stall.awaitEntered() + + fixture.onCallerThread { networkCallback.onAvailable(fixture.otherNetwork) } + stall.release() + val type = refresh.get(TIMEOUT_SECONDS, SECONDS) + + assertThat(type).isEqualTo("cellular") + } + @Test fun `getConnectionType reads a fresh cache without waiting for another caller's in-flight cache refresh`() { val fixture = Fixture.create() @@ -717,8 +737,10 @@ class AndroidConnectionStatusProviderConcurrencyTest { val buildInfo = mock() val logger = mock() val network = mock() + val otherNetwork = mock() val wifiCapabilities = capabilities(TRANSPORT_WIFI) val cellularCapabilities = capabilities(TRANSPORT_CELLULAR) + val ticker = TestMonotonicTicker() private val stalls = mutableListOf() private val callerThreads = mutableListOf() @@ -752,7 +774,7 @@ class AndroidConnectionStatusProviderConcurrencyTest { } fun createProvider(): AndroidConnectionStatusProvider = - AndroidConnectionStatusProvider(context, options, buildInfo, TestMonotonicTicker()).also { + AndroidConnectionStatusProvider(context, options, buildInfo, ticker).also { connectionStatusProvider = it } @@ -807,10 +829,10 @@ class AndroidConnectionStatusProviderConcurrencyTest { .unregisterTelephonyCallback(any()) } - fun stallNetworkCapabilitiesQuery(): Stall = + fun stallNetworkCapabilitiesQuery(result: NetworkCapabilities = wifiCapabilities): Stall = newStall().also { stall -> whenever(connectivityManager.getNetworkCapabilities(any())).thenAnswer { - stall.block(wifiCapabilities) + stall.block(result) } } @@ -877,6 +899,9 @@ class AndroidConnectionStatusProviderConcurrencyTest { /** Bounds every wait, so a regression fails the test instead of hanging the build. */ const val TIMEOUT_SECONDS = 5L + /** Longer than the provider's 2-minute cache TTL, so advancing by it makes the cache stale. */ + const val CACHE_EXPIRY_MINUTES = 3L + /** * Bounds a stalled call, which the test releases itself. Much longer than [TIMEOUT_SECONDS], so * that a caller waiting for the stalled call times out before the stall gives up. From 5c30e7f76c8b9cdd82c7b6b2fc3b7c4bbcca0f78 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Tue, 10 Nov 2026 14:01:01 +0100 Subject: [PATCH 10/10] ref(android): Add ConnectionSnapshot.toString for debugging Print the network, a summary of the capabilities the provider reads (transports, internet, validated) and whether the snapshot is still fresh. The full NetworkCapabilities isn't printed, because on API 29+ it can include the Wi-Fi SSID. Refs #6509 --- .../util/AndroidConnectionStatusProvider.java | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) 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 876c5fb48fb..62618fb084b 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 @@ -1035,6 +1035,60 @@ ConnectionSnapshot withCapabilities( final @Nullable NetworkCapabilities capabilities, final @NotNull Deadline freshUntil) { return new ConnectionSnapshot(network, capabilities, freshUntil); } + + /** + * For debugging. Summarizes the capabilities this provider reads instead of printing them in + * full because {@link NetworkCapabilities#toString()} can include the Wi-Fi SSID on API 29+. + */ + @SuppressLint("InlinedApi") + @Override + public @NotNull String toString() { + final @NotNull StringBuilder sb = new StringBuilder("ConnectionSnapshot{network="); + sb.append(network).append(", capabilities="); + + if (capabilities == null) { + sb.append("none"); + } else { + sb.append("[transports="); + boolean anyTransport = false; + + for (final int transport : transports) { + if (capabilities.hasTransport(transport)) { + sb.append(anyTransport ? "|" : "").append(transportName(transport)); + anyTransport = true; + } + } + + if (!anyTransport) { + sb.append("none"); + } + sb.append(", internet=") + .append(capabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET)) + .append(", validated=") + .append(capabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_VALIDATED)) + .append(']'); + } + + final long remainingMs = freshUntil.remaining(TimeUnit.MILLISECONDS); + sb.append(", ").append(remainingMs > 0 ? "fresh for " + remainingMs + "ms" : "stale"); + + return sb.append('}').toString(); + } + + private static @NotNull String transportName(final int transport) { + switch (transport) { + case NetworkCapabilities.TRANSPORT_WIFI: + return "wifi"; + case NetworkCapabilities.TRANSPORT_CELLULAR: + return "cellular"; + case NetworkCapabilities.TRANSPORT_ETHERNET: + return "ethernet"; + case NetworkCapabilities.TRANSPORT_BLUETOOTH: + return "bluetooth"; + default: + return String.valueOf(transport); + } + } } private void submitSafe(@NotNull Runnable r) {