From dc9cb2c2ddc37505de4237f9b62c47b72e40c5df Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Wed, 8 Sep 2021 15:24:50 +0200 Subject: [PATCH] Fix ConnectionUtil/ConnectionManager NPE (#145) --- .../java/com/splunk/rum/ConnectionUtil.java | 44 +++++------- .../main/java/com/splunk/rum/SplunkRum.java | 9 ++- .../com/splunk/rum/ConnectionUtilTest.java | 72 +++++++++++++++---- 3 files changed, 81 insertions(+), 44 deletions(-) diff --git a/splunk-otel-android/src/main/java/com/splunk/rum/ConnectionUtil.java b/splunk-otel-android/src/main/java/com/splunk/rum/ConnectionUtil.java index ca99e547..d88c62f6 100644 --- a/splunk-otel-android/src/main/java/com/splunk/rum/ConnectionUtil.java +++ b/splunk-otel-android/src/main/java/com/splunk/rum/ConnectionUtil.java @@ -16,7 +16,6 @@ package com.splunk.rum; -import android.content.Context; import android.net.ConnectivityManager; import android.net.Network; import android.net.NetworkCapabilities; @@ -26,7 +25,6 @@ import androidx.annotation.NonNull; -import java.util.concurrent.atomic.AtomicReference; import java.util.function.Supplier; //note: based on ideas from stack overflow: https://stackoverflow.com/questions/32547006/connectivitymanager-getnetworkinfoint-deprecated @@ -35,37 +33,33 @@ class ConnectionUtil { static final CurrentNetwork NO_NETWORK = new CurrentNetwork(NetworkState.NO_NETWORK_AVAILABLE, null); static final CurrentNetwork UNKNOWN_NETWORK = new CurrentNetwork(NetworkState.TRANSPORT_UNKNOWN, null); - private final ConnectionMonitor connectionMonitor; private final NetworkDetector networkDetector; - private final AtomicReference currentNetwork = new AtomicReference<>(); + private volatile CurrentNetwork currentNetwork; + private volatile ConnectionStateListener connectionStateListener; - ConnectionUtil(Context context) { - this(ConnectionUtil::createNetworkMonitoringRequest, NetworkDetector.create(context), (ConnectivityManager) context.getSystemService(Context.CONNECTIVITY_SERVICE)); + ConnectionUtil(NetworkDetector networkDetector) { + this.networkDetector = networkDetector; } - //for testing, since building the NetworkRequest fails in junit due to .. Android. - ConnectionUtil(Supplier createNetworkMonitoringRequest, NetworkDetector networkDetector, ConnectivityManager connectivityManager) { - this.connectionMonitor = new ConnectionMonitor(); - + void startMonitoring(Supplier createNetworkMonitoringRequest, ConnectivityManager connectivityManager) { + refreshNetworkStatus(); if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { - connectivityManager.registerDefaultNetworkCallback(connectionMonitor); + connectivityManager.registerDefaultNetworkCallback(new ConnectionMonitor()); } else { NetworkRequest networkRequest = createNetworkMonitoringRequest.get(); - connectivityManager.registerNetworkCallback(networkRequest, connectionMonitor); + connectivityManager.registerNetworkCallback(networkRequest, new ConnectionMonitor()); } - this.networkDetector = networkDetector; - refreshNetworkStatus(); } CurrentNetwork refreshNetworkStatus() { CurrentNetwork activeNetwork = networkDetector.detectCurrentNetwork(); - currentNetwork.set(activeNetwork); + currentNetwork = activeNetwork; return activeNetwork; } - private static NetworkRequest createNetworkMonitoringRequest() { - //todo: this throws an NPE when running in junit. what's up with that? + static NetworkRequest createNetworkMonitoringRequest() { + //note: this throws an NPE when running in junit without robolectric, due to Android return new NetworkRequest.Builder() .addTransportType(NetworkCapabilities.TRANSPORT_CELLULAR) .addTransportType(NetworkCapabilities.TRANSPORT_WIFI) @@ -76,24 +70,18 @@ private static NetworkRequest createNetworkMonitoringRequest() { } boolean isOnline() { - return currentNetwork.get().isOnline(); + return currentNetwork.isOnline(); } CurrentNetwork getActiveNetwork() { - return currentNetwork.get(); + return currentNetwork; } void setInternetStateListener(ConnectionStateListener listener) { - connectionMonitor.setOnConnectionStateListener(listener); + connectionStateListener = listener; } - class ConnectionMonitor extends ConnectivityManager.NetworkCallback { - - private ConnectionStateListener connectionStateListener; - - void setOnConnectionStateListener(ConnectionStateListener connectionStateListener) { - this.connectionStateListener = connectionStateListener; - } + private class ConnectionMonitor extends ConnectivityManager.NetworkCallback { @Override public void onAvailable(@NonNull Network network) { @@ -112,7 +100,7 @@ public void onLost(@NonNull Network network) { //this method, we'll force it to be NO_NETWORK, rather than relying on the ConnectivityManager to have the right //state at the right time during this event. CurrentNetwork activeNetwork = NO_NETWORK; - currentNetwork.set(activeNetwork); + currentNetwork = activeNetwork; if (connectionStateListener != null) { connectionStateListener.onAvailable(false, activeNetwork); Log.d(SplunkRum.LOG_TAG, " onLost: isConnected:" + false + ", activeNetwork: " + activeNetwork); diff --git a/splunk-otel-android/src/main/java/com/splunk/rum/SplunkRum.java b/splunk-otel-android/src/main/java/com/splunk/rum/SplunkRum.java index 181234da..b33bfae2 100644 --- a/splunk-otel-android/src/main/java/com/splunk/rum/SplunkRum.java +++ b/splunk-otel-android/src/main/java/com/splunk/rum/SplunkRum.java @@ -19,6 +19,8 @@ import static io.opentelemetry.api.common.AttributeKey.stringKey; import android.app.Application; +import android.content.Context; +import android.net.ConnectivityManager; import android.os.Looper; import android.util.Log; @@ -90,7 +92,12 @@ public static Config.Builder newConfigBuilder() { * @return A fully initialized {@link SplunkRum} instance, ready for use. */ public static SplunkRum initialize(Config config, Application application) { - return initialize(config, application, () -> new ConnectionUtil(application.getApplicationContext())); + return initialize(config, application, () -> { + Context context = application.getApplicationContext(); + ConnectionUtil connectionUtil = new ConnectionUtil(NetworkDetector.create(context)); + connectionUtil.startMonitoring(ConnectionUtil::createNetworkMonitoringRequest, (ConnectivityManager) context.getSystemService(Context.CONNECTIVITY_SERVICE)); + return connectionUtil; + }); } //for testing purposes diff --git a/splunk-otel-android/src/test/java/com/splunk/rum/ConnectionUtilTest.java b/splunk-otel-android/src/test/java/com/splunk/rum/ConnectionUtilTest.java index 79b8a927..c6bf0307 100644 --- a/splunk-otel-android/src/test/java/com/splunk/rum/ConnectionUtilTest.java +++ b/splunk-otel-android/src/test/java/com/splunk/rum/ConnectionUtilTest.java @@ -16,7 +16,21 @@ package com.splunk.rum; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.isA; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + import android.net.ConnectivityManager; +import android.net.ConnectivityManager.NetworkCallback; +import android.net.Network; import android.net.NetworkRequest; import android.os.Build; @@ -28,16 +42,6 @@ import java.util.concurrent.atomic.AtomicInteger; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.ArgumentMatchers.isA; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; - @RunWith(RobolectricTestRunner.class) public class ConnectionUtilTest { @@ -52,12 +56,13 @@ public void lollipop() { .thenReturn(new CurrentNetwork(NetworkState.TRANSPORT_WIFI, null)) //called on init .thenReturn(new CurrentNetwork(NetworkState.TRANSPORT_CELLULAR, "LTE")); - ConnectionUtil connectionUtil = new ConnectionUtil(() -> networkRequest, networkDetector, connectivityManager); + ConnectionUtil connectionUtil = new ConnectionUtil(networkDetector); + connectionUtil.startMonitoring(() -> networkRequest, connectivityManager); assertTrue(connectionUtil.isOnline()); assertEquals(new CurrentNetwork(NetworkState.TRANSPORT_WIFI, null), connectionUtil.getActiveNetwork()); - ArgumentCaptor monitorCaptor = ArgumentCaptor.forClass(ConnectionUtil.ConnectionMonitor.class); + ArgumentCaptor monitorCaptor = ArgumentCaptor.forClass(NetworkCallback.class); verify(connectivityManager).registerNetworkCallback(eq(networkRequest), monitorCaptor.capture()); AtomicInteger notified = new AtomicInteger(0); @@ -89,13 +94,14 @@ public void quiznos() { .thenReturn(new CurrentNetwork(NetworkState.TRANSPORT_WIFI, null)) .thenReturn(new CurrentNetwork(NetworkState.TRANSPORT_CELLULAR, "LTE")); - ConnectionUtil connectionUtil = new ConnectionUtil(() -> networkRequest, networkDetector, connectivityManager); + ConnectionUtil connectionUtil = new ConnectionUtil(networkDetector); + connectionUtil.startMonitoring(() -> networkRequest, connectivityManager); assertTrue(connectionUtil.isOnline()); assertEquals(new CurrentNetwork(NetworkState.TRANSPORT_WIFI, null), connectionUtil.getActiveNetwork()); - verify(connectivityManager, never()).registerNetworkCallback(eq(networkRequest), isA(ConnectionUtil.ConnectionMonitor.class)); + verify(connectivityManager, never()).registerNetworkCallback(eq(networkRequest), isA(NetworkCallback.class)); - ArgumentCaptor monitorCaptor = ArgumentCaptor.forClass(ConnectionUtil.ConnectionMonitor.class); + ArgumentCaptor monitorCaptor = ArgumentCaptor.forClass(NetworkCallback.class); verify(connectivityManager).registerDefaultNetworkCallback(monitorCaptor.capture()); AtomicInteger notified = new AtomicInteger(0); @@ -115,4 +121,40 @@ public void quiznos() { monitorCaptor.getValue().onLost(null); assertEquals(2, notified.get()); } + + @Test + @Config(sdk = Build.VERSION_CODES.LOLLIPOP) + public void shouldNotFailOnImmediateConnectionManagerCall_lollipop() { + NetworkRequest networkRequest = mock(NetworkRequest.class); + NetworkDetector networkDetector = mock(NetworkDetector.class); + ConnectivityManager connectivityManager = mock(ConnectivityManager.class); + + doAnswer(invocation -> { + NetworkCallback callback = invocation.getArgument(1); + callback.onAvailable(mock(Network.class)); + return null; + }).when(connectivityManager) + .registerNetworkCallback(eq(networkRequest), any(NetworkCallback.class)); + + ConnectionUtil connectionUtil = new ConnectionUtil(networkDetector); + connectionUtil.startMonitoring(() -> networkRequest, connectivityManager); + } + + @Test + @Config(sdk = Build.VERSION_CODES.Q) + public void shouldNotFailOnImmediateConnectionManagerCall_quiznos() { + NetworkRequest networkRequest = mock(NetworkRequest.class); + NetworkDetector networkDetector = mock(NetworkDetector.class); + ConnectivityManager connectivityManager = mock(ConnectivityManager.class); + + doAnswer(invocation -> { + NetworkCallback callback = invocation.getArgument(0); + callback.onAvailable(mock(Network.class)); + return null; + }).when(connectivityManager) + .registerDefaultNetworkCallback(any(NetworkCallback.class)); + + ConnectionUtil connectionUtil = new ConnectionUtil(networkDetector); + connectionUtil.startMonitoring(() -> networkRequest, connectivityManager); + } } \ No newline at end of file