Skip to content

Commit bc2bd59

Browse files
alwxalwx
committed
ref(android): Address the review of the cellular technology
* `CellularNetworkTechnologyProvider` guards its registration with `synchronized` instead of `AutoClosableReentrantLock`. That lock only exists for a JVM bug, and this code is Android only. * The provider takes the executor service and builds the executor the telephony framework calls, with the guard against a rejection. The constructor of `AndroidConnectionStatusProvider` no longer sets up that exception handling. * `Connection` moves out of `AndroidConnectionStatusProvider`, which is already a large class, into a top level `NetworkConnection`. The name is not `ConnectionType`, because the class holds the connection type and the effective type. * The test for the rejected executor is removed, together with its executor double.
1 parent 5a32881 commit bc2bd59

6 files changed

Lines changed: 61 additions & 94 deletions

File tree

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import io.sentry.android.core.internal.util.AndroidConnectionStatusProvider;
2222
import io.sentry.android.core.internal.util.CpuInfoUtils;
2323
import io.sentry.android.core.internal.util.DeviceOrientations;
24+
import io.sentry.android.core.internal.util.NetworkConnection;
2425
import io.sentry.android.core.internal.util.RootChecker;
2526
import io.sentry.protocol.Device;
2627
import io.sentry.protocol.OperatingSystem;
@@ -234,7 +235,7 @@ private void setDeviceIO(
234235
options.getConnectionStatusProvider();
235236
if (connectionStatusProvider instanceof AndroidConnectionStatusProvider) {
236237
// Reading both at once keeps the type and the technology describing the same network.
237-
final AndroidConnectionStatusProvider.Connection connection =
238+
final NetworkConnection connection =
238239
((AndroidConnectionStatusProvider) connectionStatusProvider).getConnection();
239240
// wifi, ethernet or cellular, null if none
240241
device.setConnectionType(connection.type);

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

Lines changed: 4 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424
import io.sentry.util.AutoClosableReentrantLock;
2525
import java.util.ArrayList;
2626
import java.util.List;
27-
import java.util.concurrent.RejectedExecutionException;
2827
import java.util.concurrent.TimeUnit;
2928
import java.util.concurrent.atomic.AtomicBoolean;
3029
import org.jetbrains.annotations.ApiStatus;
@@ -98,24 +97,7 @@ public AndroidConnectionStatusProvider(
9897
this.connectionStatusObservers = new ArrayList<>();
9998
this.cellularNetworkTechnologyProvider =
10099
new CellularNetworkTechnologyProvider(
101-
this.context,
102-
options.getLogger(),
103-
buildInfoProvider,
104-
runnable -> {
105-
try {
106-
options.getExecutorService().submit(runnable);
107-
} catch (RejectedExecutionException e) {
108-
// The telephony framework calls this executor, so an exception would be thrown on
109-
// one of its threads. The SDK's executor rejects work once it is shut down, which
110-
// can happen before the listener is unregistered.
111-
options
112-
.getLogger()
113-
.log(
114-
SentryLevel.DEBUG,
115-
"Dropping a cellular network technology update, the executor rejected it.",
116-
e);
117-
}
118-
});
100+
this.context, options.getLogger(), buildInfoProvider, options.getExecutorService());
119101

120102
capabilities[0] = NetworkCapabilities.NET_CAPABILITY_INTERNET;
121103
if (buildInfoProvider.getSdkInfoVersion() >= Build.VERSION_CODES.M) {
@@ -198,35 +180,18 @@ private boolean isNetworkEffectivelyConnected(
198180
* <p>Both are derived from one cache read, so they always describe the same network instead of
199181
* straddling a connectivity change.
200182
*/
201-
public @NotNull Connection getConnection() {
183+
public @NotNull NetworkConnection getConnection() {
202184
if (!isCacheValid()) {
203185
updateCache(null);
204186
}
205187
final @Nullable String connectionType = getConnectionTypeFromCache();
206188
if (!"cellular".equals(connectionType)) {
207-
return new Connection(connectionType, null);
189+
return new NetworkConnection(connectionType, null);
208190
}
209-
return new Connection(
191+
return new NetworkConnection(
210192
connectionType, cellularNetworkTechnologyProvider.getCellularNetworkTechnology());
211193
}
212194

213-
/** The connection type and the generation of the cellular network technology, if any. */
214-
public static final class Connection {
215-
/** Maps to the {@code network.connection.type} attribute, {@code null} when unknown. */
216-
public final @Nullable String type;
217-
218-
/**
219-
* Maps to the {@code network.connection.effective_type} attribute, for example {@code 5g}.
220-
* {@code null} when the connection is not cellular or the technology is unknown.
221-
*/
222-
public final @Nullable String effectiveType;
223-
224-
Connection(final @Nullable String type, final @Nullable String effectiveType) {
225-
this.type = type;
226-
this.effectiveType = effectiveType;
227-
}
228-
}
229-
230195
private void ensureNetworkCallbackRegistered() {
231196
if (!ContextUtils.isForegroundImportance()) {
232197
return;

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

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,12 @@
1010
import androidx.annotation.NonNull;
1111
import androidx.annotation.RequiresApi;
1212
import io.sentry.ILogger;
13-
import io.sentry.ISentryLifecycleToken;
13+
import io.sentry.ISentryExecutorService;
1414
import io.sentry.SentryLevel;
1515
import io.sentry.android.core.BuildInfoProvider;
1616
import io.sentry.android.core.ContextUtils;
17-
import io.sentry.util.AutoClosableReentrantLock;
1817
import java.util.concurrent.Executor;
18+
import java.util.concurrent.RejectedExecutionException;
1919
import org.jetbrains.annotations.ApiStatus;
2020
import org.jetbrains.annotations.NotNull;
2121
import org.jetbrains.annotations.Nullable;
@@ -41,8 +41,16 @@ public final class CellularNetworkTechnologyProvider {
4141
private final @NotNull Context context;
4242
private final @NotNull ILogger logger;
4343
private final @NotNull BuildInfoProvider buildInfoProvider;
44+
45+
/** Guards the registration of the display info listener. */
46+
private final @NotNull Object registrationLock = new Object();
47+
48+
/**
49+
* The executor the telephony framework calls. The framework calls it on a thread of its own, so a
50+
* rejection must not escape: the SDK's executor rejects work once it is shut down, which can
51+
* happen before the listener is unregistered.
52+
*/
4453
private final @NotNull Executor executor;
45-
private final @NotNull AutoClosableReentrantLock lock = new AutoClosableReentrantLock();
4654

4755
/**
4856
* Set from {@link TelephonyCallback.DisplayInfoListener} on API 31 and above. Declared as {@link
@@ -57,11 +65,21 @@ public CellularNetworkTechnologyProvider(
5765
final @NotNull Context context,
5866
final @NotNull ILogger logger,
5967
final @NotNull BuildInfoProvider buildInfoProvider,
60-
final @NotNull Executor executor) {
68+
final @NotNull ISentryExecutorService executorService) {
6169
this.context = ContextUtils.getApplicationContext(context);
6270
this.logger = logger;
6371
this.buildInfoProvider = buildInfoProvider;
64-
this.executor = executor;
72+
this.executor =
73+
runnable -> {
74+
try {
75+
executorService.submit(runnable);
76+
} catch (RejectedExecutionException e) {
77+
logger.log(
78+
SentryLevel.DEBUG,
79+
"Dropping a cellular network technology update, the executor rejected it.",
80+
e);
81+
}
82+
};
6583
}
6684

6785
/**
@@ -86,7 +104,7 @@ public void register() {
86104
}
87105
// Checking and storing the callback has to be atomic, otherwise concurrent callers can each
88106
// register a listener while only the last one is kept and can ever be unregistered.
89-
try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) {
107+
synchronized (registrationLock) {
90108
if (displayInfoCallback != null) {
91109
return;
92110
}
@@ -118,7 +136,7 @@ public void register() {
118136
*/
119137
@SuppressLint("NewApi")
120138
public void unregister() {
121-
try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) {
139+
synchronized (registrationLock) {
122140
final @Nullable Object callback = displayInfoCallback;
123141
if (callback == null) {
124142
return;

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
package io.sentry.android.core.internal.util;
2+
3+
import org.jetbrains.annotations.ApiStatus;
4+
import org.jetbrains.annotations.Nullable;
5+
6+
/** The connection type and the generation of the cellular network technology, if any. */
7+
@ApiStatus.Internal
8+
public final class NetworkConnection {
9+
/** Maps to the {@code network.connection.type} attribute, {@code null} when unknown. */
10+
public final @Nullable String type;
11+
12+
/**
13+
* Maps to the {@code network.connection.effective_type} attribute, for example {@code 5g}. {@code
14+
* null} when the connection is not cellular or the technology is unknown.
15+
*/
16+
public final @Nullable String effectiveType;
17+
18+
NetworkConnection(final @Nullable String type, final @Nullable String effectiveType) {
19+
this.type = type;
20+
this.effectiveType = effectiveType;
21+
}
22+
}

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

Lines changed: 0 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -21,17 +21,13 @@ import android.telephony.TelephonyManager
2121
import androidx.test.ext.junit.runners.AndroidJUnit4
2222
import io.sentry.IConnectionStatusProvider
2323
import io.sentry.ILogger
24-
import io.sentry.ISentryExecutorService
2524
import io.sentry.SentryOptions
2625
import io.sentry.android.core.AppState
2726
import io.sentry.android.core.BuildInfoProvider
2827
import io.sentry.android.core.ContextUtils
2928
import io.sentry.android.core.SystemEventsBreadcrumbsIntegration
3029
import io.sentry.test.ImmediateExecutorService
3130
import io.sentry.time.TestMonotonicTicker
32-
import java.util.concurrent.Executor
33-
import java.util.concurrent.Future
34-
import java.util.concurrent.RejectedExecutionException
3531
import java.util.concurrent.TimeUnit.MINUTES
3632
import kotlin.test.AfterTest
3733
import kotlin.test.BeforeTest
@@ -259,43 +255,6 @@ class AndroidConnectionStatusProviderTest {
259255
}
260256

261257
/// Lets a test reject work only after the setup that needs a working executor is done.
262-
private class RejectingExecutorService(private val delegate: ISentryExecutorService) :
263-
ISentryExecutorService by delegate {
264-
var reject = false
265-
266-
override fun submit(runnable: Runnable): Future<*> {
267-
if (reject) {
268-
throw RejectedExecutionException("closed")
269-
}
270-
return delegate.submit(runnable)
271-
}
272-
}
273-
274-
@Test
275-
@Config(sdk = [Build.VERSION_CODES.S])
276-
fun `When the executor rejects work, a technology update does not throw`() {
277-
// The telephony framework calls the executor, so throwing there would surface on one of its
278-
// threads. The SDK executor rejects work once it is shut down.
279-
val telephonyManager = mock<TelephonyManager>()
280-
whenever(contextMock.getSystemService(eq(Context.TELEPHONY_SERVICE)))
281-
.thenReturn(telephonyManager)
282-
whenever(buildInfo.sdkInfoVersion).thenReturn(Build.VERSION_CODES.S)
283-
val executor = RejectingExecutorService(ImmediateExecutorService())
284-
options.executorService = executor
285-
286-
val provider = AndroidConnectionStatusProvider(contextMock, options, buildInfo, ticker)
287-
val captor = argumentCaptor<Executor>()
288-
verify(telephonyManager).registerTelephonyCallback(captor.capture(), any())
289-
executor.reject = true
290-
291-
captor.firstValue.execute {}
292-
293-
// Cleanup runs through the same executor, and connectivityManager is static, so a still
294-
// rejecting executor would leak a stale manager into later tests.
295-
executor.reject = false
296-
provider.close()
297-
}
298-
299258
@Test
300259
@Config(sdk = [Build.VERSION_CODES.S])
301260
fun `When the network callback cannot be registered, the telephony listener is not started`() {

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

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@ import androidx.test.ext.junit.runners.AndroidJUnit4
1111
import com.google.common.truth.Truth.assertThat
1212
import io.sentry.ILogger
1313
import io.sentry.android.core.BuildInfoProvider
14+
import io.sentry.test.ImmediateExecutorService
1415
import java.util.concurrent.CountDownLatch
15-
import java.util.concurrent.Executor
1616
import java.util.concurrent.TimeUnit
1717
import kotlin.test.BeforeTest
1818
import kotlin.test.Test
@@ -37,7 +37,7 @@ class CellularNetworkTechnologyProviderTest {
3737
val telephonyManager = mock<TelephonyManager>()
3838

3939
/** Runs the display info callback inline, so tests don't have to wait for another thread. */
40-
val executor = Executor { it.run() }
40+
val executorService = ImmediateExecutorService()
4141

4242
fun getSut(
4343
sdkVersion: Int = Build.VERSION_CODES.S,
@@ -49,7 +49,7 @@ class CellularNetworkTechnologyProviderTest {
4949
.thenReturn(if (hasTelephonyManager) telephonyManager else null)
5050
whenever(context.checkPermission(any(), any(), any()))
5151
.thenReturn(if (hasPermission) PERMISSION_GRANTED else PERMISSION_DENIED)
52-
return CellularNetworkTechnologyProvider(context, logger, buildInfo, executor)
52+
return CellularNetworkTechnologyProvider(context, logger, buildInfo, executorService)
5353
}
5454

5555
fun displayInfo(networkType: Int, overrideNetworkType: Int): TelephonyDisplayInfo {
@@ -65,7 +65,9 @@ class CellularNetworkTechnologyProviderTest {
6565
): TelephonyCallback.DisplayInfoListener {
6666
provider.register()
6767
val captor = argumentCaptor<TelephonyCallback>()
68-
verify(telephonyManager).registerTelephonyCallback(eq(executor), captor.capture())
68+
// The provider wraps the executor service, so the executor it hands to the telephony
69+
// manager is not an object this test holds.
70+
verify(telephonyManager).registerTelephonyCallback(any(), captor.capture())
6971
return captor.firstValue as TelephonyCallback.DisplayInfoListener
7072
}
7173
}
@@ -246,7 +248,7 @@ class CellularNetworkTechnologyProviderTest {
246248
provider.register()
247249
provider.register()
248250

249-
verify(fixture.telephonyManager).registerTelephonyCallback(eq(fixture.executor), any())
251+
verify(fixture.telephonyManager).registerTelephonyCallback(any(), any())
250252
}
251253

252254
@Test
@@ -267,7 +269,7 @@ class CellularNetworkTechnologyProviderTest {
267269
start.countDown()
268270
assertThat(done.await(10, TimeUnit.SECONDS)).isTrue()
269271

270-
verify(fixture.telephonyManager).registerTelephonyCallback(eq(fixture.executor), any())
272+
verify(fixture.telephonyManager).registerTelephonyCallback(any(), any())
271273
}
272274

273275
@Test

0 commit comments

Comments
 (0)