diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a08fb1ae6c..8680b4ceb62 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 ([#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)) 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..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 @@ -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; @@ -22,19 +23,62 @@ 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 java.util.concurrent.atomic.AtomicReference; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; 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: + * + *

+ * + * 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: + * + *

*/ @ApiStatus.Internal public final class AndroidConnectionStatusProvider @@ -44,19 +88,45 @@ public final class AndroidConnectionStatusProvider private final @NotNull SentryOptions options; private final @NotNull BuildInfoProvider buildInfoProvider; private final @NotNull MonotonicTicker ticker; - private final @NotNull List connectionStatusObservers; + + /** + * 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 = + new CopyOnWriteArrayList<>(); + private final @NotNull CellularNetworkTechnologyProvider cellularNetworkTechnologyProvider; private final @Nullable Handler handler; - private final @NotNull AutoClosableReentrantLock lock = new AutoClosableReentrantLock(); + + /** + * 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; 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<>(); + /** + * 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<>(); private static final int[] transports = { NetworkCapabilities.TRANSPORT_WIFI, @@ -65,13 +135,44 @@ public final class AndroidConnectionStatusProvider NetworkCapabilities.TRANSPORT_BLUETOOTH }; - private static final int[] capabilities = new int[2]; + /** + * 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; - private volatile @Nullable NetworkCapabilities cachedNetworkCapabilities; - private volatile @Nullable Network currentNetwork; - private volatile @NotNull Deadline cacheFreshUntil; 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); + + /** + * 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 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; + + /** + * 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, @@ -92,16 +193,18 @@ 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 ArrayList<>(); 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 @@ -144,10 +247,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) + /** + * 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; + if (capabilities != null) { + return isNetworkEffectivelyConnected(capabilities) ? ConnectionStatus.CONNECTED : ConnectionStatus.DISCONNECTED; } @@ -162,9 +269,14 @@ 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; + /** + * 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; if (capabilities != null) { return getConnectionType(capabilities); } @@ -175,15 +287,9 @@ 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. */ 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); } @@ -192,6 +298,10 @@ private boolean isNetworkEffectivelyConnected( } private void ensureNetworkCallbackRegistered() { + if (closed) { + return; + } + if (!ContextUtils.isForegroundImportance()) { return; } @@ -200,8 +310,11 @@ private void ensureNetworkCallbackRegistered() { return; // Already registered } - try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { - if (networkCallback != null) { + final boolean registered; + try (final @NotNull ISentryLifecycleToken ignored = registrationLock.acquire()) { + // 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; } @@ -209,16 +322,16 @@ private void ensureNetworkCallbackRegistered() { new NetworkCallback() { @Override public void onAvailable(final @NotNull Network network) { - currentNetwork = 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)) { - try (final @NotNull ISentryLifecycleToken ignored = childCallbacksLock.acquire()) { - for (final @NotNull NetworkCallback cb : childCallbacks) { - cb.onAvailable(network); - } + @NotNull ConnectionSnapshot current; + do { + current = snapshot.get(); + } while (!snapshot.compareAndSet(current, current.withNetwork(network))); + + // 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); } } } @@ -228,79 +341,75 @@ 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(); } } @Override public void onLost(final @NotNull Network network) { - if (!network.equals(currentNetwork)) { + if (!network.equals(snapshot.get().network)) { return; } 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); } } 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); - - 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); - } + onAvailableForwarded.set(false); + snapshot.set(ConnectionSnapshot.lost(ticker)); + + 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); } } @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); - 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); } } + /** + * 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( - @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(); - try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { - 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); } } @@ -308,19 +417,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; @@ -338,9 +442,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; } } @@ -362,121 +465,147 @@ 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; - } + /** 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); + } - if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.M) { - cachedNetworkCapabilities = null; - cacheFreshUntil = Deadline.after(ticker, CACHE_TTL_MINUTES, TimeUnit.MINUTES); - return; - } + /** + * 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 the fresh snapshot another thread stored during the query + */ + private @NotNull ConnectionSnapshot refresh(final @NotNull ConnectionSnapshot before) { + 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; + } + } - // 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 - } - } - cacheFreshUntil = Deadline.after(ticker, CACHE_TTL_MINUTES, TimeUnit.MINUTES); + // 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; + } + /** + * Queries the capabilities of the active network from the {@link ConnectivityManager}. + * + * @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() { + 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; } - } - } - private boolean isCacheValid() { - return !cacheFreshUntil.hasPassed(); + 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; + } } @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()); } + /** + * 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'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 + */ @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); + // 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()); + 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; @@ -484,10 +613,7 @@ private void unregisterNetworkCallback(final boolean clearObservers) { unregisterNetworkCallback(context, options.getLogger(), callbackRef); } cellularNetworkTechnologyProvider.unregister(); - // Clear cached state - cachedNetworkCapabilities = null; - currentNetwork = null; - cacheFreshUntil = Deadline.passed(ticker); + snapshot.set(ConnectionSnapshot.unknown(ticker)); } options.getLogger().log(SentryLevel.DEBUG, "Network callback unregistered"); } @@ -495,12 +621,12 @@ 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); - try (final @NotNull ISentryLifecycleToken ignored = childCallbacksLock.acquire()) { - childCallbacks.clear(); - } + childCallbacks.clear(); try (final @NotNull ISentryLifecycleToken ignored = connectivityManagerLock.acquire()) { connectivityManager = null; } @@ -508,66 +634,72 @@ public void close() { }); } + /** + * 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() { - if (networkCallback != null) { - return; - } - 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; + } + // 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 - isConnected.set(false); - try (final @NotNull ISentryLifecycleToken ignored = childCallbacksLock.acquire()) { - for (final @NotNull NetworkCallback cb : childCallbacks) { - //noinspection DataFlowIssue - cb.onLost(null); - } + onAvailableForwarded.set(false); + 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 - // it won't updateCache and notify observes because we just did it above and the cached - // capabilities will be the same + // it won't refresh the cache and notify observers because we just did it above and the + // cached capabilities will be the same ensureNetworkCallbackRegistered(); }); } + /** + * Schedules unregistration of the network callback and the cellular technology listener, if + * registered. + */ @Override public void onBackground() { - if (networkCallback == null) { - return; - } - submitSafe( () -> { - //noinspection Convert2MethodRef + // 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; + } unregisterNetworkCallback(/* clearObservers= */ false); }); } /** - * 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 */ @TestOnly @Nullable public NetworkCapabilities getCachedNetworkCapabilities() { - return cachedNetworkCapabilities; + return snapshot.get().capabilities; } /** @@ -768,16 +900,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"}) @@ -847,6 +975,122 @@ public static List getChildCallbacks() { return childCallbacks; } + /** + * An atomically publishable snapshot of cached network connection info, obtained from {@link + * ConnectivityManager}. + * + *

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 { + + /** Info about the network of interest, or {@code null} if nothing is known. */ + private final @Nullable Network network; + + /** + * The capabilities of the network. {@code null} if they're unknown or there's no known network. + */ + private final @Nullable NetworkCapabilities capabilities; + + /** When this snapshot becomes stale and {@link ConnectivityManager} should be requeried. */ + private 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; + } + + /** 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)); + } + + /** 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)); + } + + /** 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) { + 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) { 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 new file mode 100644 index 00000000000..d201d48d111 --- /dev/null +++ b/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderConcurrencyTest.kt @@ -0,0 +1,916 @@ +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 com.google.common.truth.Truth.assertWithMessage +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.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.MINUTES +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.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 +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 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 status = fixture.onCallerThread { connectionStatusProvider.connectionStatus } + + assertThat(status).isEqualTo(ConnectionStatus.CONNECTED) + } + + @Test + 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() + 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 `removeConnectionStatusObserver does not wait for a stalled telephony callback registration`() { + val fixture = Fixture.create(sdkVersion = Build.VERSION_CODES.S) + 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) } + + assertThat(connectionStatusProvider.statusObservers).isEmpty() + } + + @Test + 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() + 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 an empty 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 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() + connectionStatusProvider.onBackground() + stall.awaitEntered() + + val status = fixture.onCallerThread { connectionStatusProvider.connectionStatus } + + assertThat(status).isEqualTo(ConnectionStatus.CONNECTED) + } + + // An in-flight cache refresh racing a network callback + + @Test + 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.connectionType } + stall.awaitEntered() + + fixture.onCallerThread { + networkCallback.onCapabilitiesChanged(fixture.network, fixture.cellularCapabilities) + } + stall.release() + val type = refresh.get(TIMEOUT_SECONDS, SECONDS) + + assertThat(type).isEqualTo("cellular") + } + + @Test + fun `a network loss 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 } + stall.awaitEntered() + + fixture.onCallerThread { networkCallback.onLost(fixture.network) } + stall.release() + refresh.get(TIMEOUT_SECONDS, SECONDS) + + assertThat(connectionStatusProvider.cachedNetworkCapabilities).isNull() + } + + @Test + 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() + 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) + } + + @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() + 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 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() + 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) + } + } + ) + val notification = fixture.startOnCallerThread { networkCallback.onAvailable(fixture.network) } + stall.awaitEntered() + val childNetworkCallback = mock() + + val added = fixture.onCallerThread { + AndroidConnectionStatusProvider.addNetworkCallback( + fixture.context, + fixture.logger, + fixture.buildInfo, + childNetworkCallback, + ) + } + stall.release() + notification.get(TIMEOUT_SECONDS, SECONDS) + + 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) + 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 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() + 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()) + } + + @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() + executor.runAll() + + 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() + executor.runAll() + + 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() + executor.runAll() + + 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) + 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 otherNetwork = mock() + val wifiCapabilities = capabilities(TRANSPORT_WIFI) + val cellularCapabilities = capabilities(TRANSPORT_CELLULAR) + val ticker = TestMonotonicTicker() + 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, ticker).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(result: NetworkCapabilities = wifiCapabilities): Stall = + newStall().also { stall -> + whenever(connectivityManager.getNetworkCapabilities(any())).thenAnswer { + stall.block(result) + } + } + + 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)) } + val hungCallerThreads = callerThreads.count { it.isAlive } + connectionStatusProvider?.close() + when (executorService) { + is SingleThreadExecutorService -> executorService.shutdown() + is DeferredExecutorService -> executorService.runAll() + } + contextUtils.close() + AppState.getInstance().unregisterLifecycleObserver() + assertWithMessage("Caller threads still running after their stalls were released") + .that(hungCallerThreads) + .isEqualTo(0) + } + + companion object { + private val created = mutableListOf() + + fun create( + sdkVersion: Int = Build.VERSION_CODES.N, + executorService: ISentryExecutorService = SingleThreadExecutorService(), + ): Fixture = Fixture(sdkVersion, executorService).also { created += it } + + fun closeAll() { + created.forEach { it.close() } + created.clear() + } + + 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 + + /** 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. + */ + 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..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 @@ -19,8 +19,10 @@ 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.SentryLevel import io.sentry.SentryOptions import io.sentry.android.core.AppState import io.sentry.android.core.BuildInfoProvider @@ -379,6 +381,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) @@ -400,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