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: + * + *
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: + * + *
Used by SDK integrations, including Session Replay and integrations that send cached
+ * envelopes.
+ */
+ private final @NotNull List 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 Used by SDK integrations, including network breadcrumbs.
+ */
+ private static final @NotNull List 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 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 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