Merge "Fix the logic for handling network status change" am: f9a72a813d am: 74be4cd836 am: 6a53d96ea2
Original change: https://android-review.googlesource.com/c/platform/packages/services/Mms/+/1739413
Change-Id: I45ef44c87c8352912f9cb2e6590d36d11efc67bb
diff --git a/src/com/android/mms/service/MmsNetworkManager.java b/src/com/android/mms/service/MmsNetworkManager.java
index af4d8a4..670ed81 100644
--- a/src/com/android/mms/service/MmsNetworkManager.java
+++ b/src/com/android/mms/service/MmsNetworkManager.java
@@ -32,8 +32,8 @@
import android.telephony.SubscriptionManager;
import android.telephony.TelephonyManager;
+import com.android.internal.annotations.VisibleForTesting;
import com.android.internal.telephony.PhoneConstants;
-
import com.android.mms.service.exception.MmsNetworkException;
/**
@@ -90,6 +90,8 @@
// If ACTION_SIM_CARD_STATE_CHANGED intent receiver is registered
private boolean mReceiverRegistered;
+ private final Dependencies mDeps;
+
/**
* This receiver listens to ACTION_SIM_CARD_STATE_CHANGED after starting a new NetworkRequest.
* If ACTION_SIM_CARD_STATE_CHANGED with SIM_STATE_ABSENT for a SIM card corresponding to the
@@ -142,22 +144,12 @@
*/
private class NetworkRequestCallback extends ConnectivityManager.NetworkCallback {
@Override
- public void onAvailable(Network network) {
- super.onAvailable(network);
- LogUtil.i("NetworkCallbackListener.onAvailable: network=" + network);
- synchronized (MmsNetworkManager.this) {
- mNetwork = network;
- MmsNetworkManager.this.notifyAll();
- }
- }
-
- @Override
public void onLost(Network network) {
super.onLost(network);
LogUtil.w("NetworkCallbackListener.onLost: network=" + network);
synchronized (MmsNetworkManager.this) {
- releaseRequestLocked(this);
- MmsNetworkManager.this.notifyAll();
+ // Wait for other available network. Not notify.
+ if (network.equals(mNetwork)) mNetwork = null;
}
}
@@ -170,10 +162,61 @@
MmsNetworkManager.this.notifyAll();
}
}
+
+ @Override
+ public void onCapabilitiesChanged(Network network, NetworkCapabilities nc) {
+ // onAvailable will always immediately be followed by a onCapabilitiesChanged. Check
+ // network status here is enough.
+ super.onCapabilitiesChanged(network, nc);
+ LogUtil.w("NetworkCallbackListener.onCapabilitiesChanged: network="
+ + network + ", nc=" + nc);
+ synchronized (MmsNetworkManager.this) {
+ final boolean isAvailable =
+ nc.hasCapability(NetworkCapabilities.NET_CAPABILITY_NOT_SUSPENDED);
+ if (network.equals(mNetwork) && !isAvailable) {
+ // Current network becomes suspended.
+ mNetwork = null;
+ // Not notify. Either wait for other available network or current network to
+ // become available again.
+ return;
+ }
+
+ // New available network
+ if (mNetwork == null && isAvailable) {
+ mNetwork = network;
+ MmsNetworkManager.this.notifyAll();
+ }
+ }
+ }
}
- public MmsNetworkManager(Context context, int subId) {
+ /**
+ * Dependencies of MmsNetworkManager, for injection in tests.
+ */
+ @VisibleForTesting
+ public static class Dependencies {
+ /** Get phone Id from the given subId */
+ public int getPhoneId(int subId) {
+ return SubscriptionManager.getPhoneId(subId);
+ }
+
+ // Timeout used to call ConnectivityManager.requestNetwork. Given that the telephony layer
+ // will retry on failures, this timeout should be high enough.
+ public int getNetworkRequestTimeoutMillis() {
+ return DeviceConfig.getInt(
+ DeviceConfig.NAMESPACE_TELEPHONY, MMS_SERVICE_NETWORK_REQUEST_TIMEOUT_MILLIS,
+ DEFAULT_MMS_SERVICE_NETWORK_REQUEST_TIMEOUT_MILLIS);
+ }
+
+ public int getAdditionalNetworkAcquireTimeoutMillis() {
+ return ADDITIONAL_NETWORK_ACQUIRE_TIMEOUT_MILLIS;
+ }
+ }
+
+ @VisibleForTesting
+ protected MmsNetworkManager(Context context, int subId, Dependencies dependencies) {
mContext = context;
+ mDeps = dependencies;
mNetworkCallback = null;
mNetwork = null;
mMmsRequestCount = 0;
@@ -200,6 +243,10 @@
};
}
+ public MmsNetworkManager(Context context, int subId) {
+ this(context, subId, new Dependencies());
+ }
+
/**
* Acquire the MMS network
*
@@ -207,7 +254,7 @@
* @throws com.android.mms.service.exception.MmsNetworkException if we fail to acquire it
*/
public void acquireNetwork(final String requestId) throws MmsNetworkException {
- int networkRequestTimeoutMillis = getNetworkRequestTimeoutMillis();
+ int networkRequestTimeoutMillis = mDeps.getNetworkRequestTimeoutMillis();
synchronized (this) {
// Since we are acquiring the network, remove the network release task if exists.
@@ -220,7 +267,7 @@
}
// Not available, so start a new request if not done yet
if (mNetworkCallback == null) {
- mPhoneId = SubscriptionManager.getPhoneId(mSubId);
+ mPhoneId = mDeps.getPhoneId(mSubId);
if (mPhoneId == SubscriptionManager.INVALID_PHONE_INDEX
|| mPhoneId == SubscriptionManager.DEFAULT_PHONE_INDEX) {
throw new MmsNetworkException("Invalid Phone Id: " + mPhoneId);
@@ -236,7 +283,8 @@
mReceiverRegistered = true;
}
try {
- this.wait(networkRequestTimeoutMillis + ADDITIONAL_NETWORK_ACQUIRE_TIMEOUT_MILLIS);
+ this.wait(networkRequestTimeoutMillis
+ + mDeps.getAdditionalNetworkAcquireTimeoutMillis());
} catch (InterruptedException e) {
LogUtil.w(requestId, "MmsNetworkManager: acquire network wait interrupted");
}
@@ -269,15 +317,6 @@
}
}
- // Timeout used to call ConnectivityManager.requestNetwork
- // Given that the telephony layer will retry on failures, this timeout should be high enough.
- private int getNetworkRequestTimeoutMillis() {
- return DeviceConfig.getInt(
- DeviceConfig.NAMESPACE_TELEPHONY, MMS_SERVICE_NETWORK_REQUEST_TIMEOUT_MILLIS,
- DEFAULT_MMS_SERVICE_NETWORK_REQUEST_TIMEOUT_MILLIS);
- }
-
-
/**
* Release the MMS network when nobody is holding on to it.
*
diff --git a/tests/robotests/src/com/android/mms/service/MmsNetworkManagerTest.java b/tests/robotests/src/com/android/mms/service/MmsNetworkManagerTest.java
new file mode 100644
index 0000000..c2d3e05
--- /dev/null
+++ b/tests/robotests/src/com/android/mms/service/MmsNetworkManagerTest.java
@@ -0,0 +1,235 @@
+/*
+ * Copyright (C) 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.mms.service;
+
+import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_SUSPENDED;
+
+import static junit.framework.Assert.assertEquals;
+import static junit.framework.Assert.assertFalse;
+import static junit.framework.Assert.fail;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.timeout;
+import static org.mockito.Mockito.verify;
+
+import android.content.Context;
+import android.net.ConnectivityManager;
+import android.net.ConnectivityManager.NetworkCallback;
+import android.net.Network;
+import android.net.NetworkCapabilities;
+import android.net.NetworkInfo;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+import org.robolectric.RobolectricTestRunner;
+
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+
+@RunWith(RobolectricTestRunner.class)
+public final class MmsNetworkManagerTest {
+ private static final int TEST_SUBID = 1234;
+ private static final int CALLBACK_TIMEOUT_MS = 3000;
+ private static final int NETWORK_ACQUIRE_TIMEOUT_MS = 5000;
+
+ private static final String MMS_APN = "mmsapn";
+ private static final String MMS_APN2 = "mmsapn2";
+ private static final NetworkCapabilities SUSPEND_NC = new NetworkCapabilities.Builder().build();
+ private static final NetworkCapabilities USABLE_NC =
+ new NetworkCapabilities.Builder().addCapability(NET_CAPABILITY_NOT_SUSPENDED).build();
+
+ @Mock Network mTestNetwork;
+ @Mock Network mTestNetwork2;
+ @Mock NetworkInfo mNetworkInfo;
+ @Mock NetworkInfo mNetworkInfo2;
+ @Mock Context mCtx;
+ @Mock ConnectivityManager mCm;
+ @Mock MmsNetworkManager.Dependencies mDeps;
+
+
+ private MmsNetworkManager mMnm;
+ private final ExecutorService mExecutor = Executors.newSingleThreadExecutor();
+ private final AtomicInteger mRequestId = new AtomicInteger(1);
+
+ @Before
+ public void setup() {
+ MockitoAnnotations.initMocks(this);
+
+ doReturn(mCm).when(mCtx).getSystemService(Context.CONNECTIVITY_SERVICE);
+ doReturn(mCm).when(mCtx).getSystemService(ConnectivityManager.class);
+ doReturn(1).when(mDeps).getPhoneId(anyInt());
+ doReturn(mNetworkInfo).when(mCm).getNetworkInfo(eq(mTestNetwork));
+ doReturn(MMS_APN).when(mNetworkInfo).getExtraInfo();
+ doReturn(mNetworkInfo2).when(mCm).getNetworkInfo(eq(mTestNetwork2));
+ doReturn(MMS_APN2).when(mNetworkInfo2).getExtraInfo();
+ doReturn(NETWORK_ACQUIRE_TIMEOUT_MS).when(mDeps).getNetworkRequestTimeoutMillis();
+ doReturn(NETWORK_ACQUIRE_TIMEOUT_MS).when(mDeps).getAdditionalNetworkAcquireTimeoutMillis();
+
+ mMnm = new MmsNetworkManager(mCtx, TEST_SUBID, mDeps);
+ }
+
+ @Test
+ public void testAvailableNetwork_newNetworkAvailable() throws Exception {
+ acquireAvailableNetworkAndGetCallback(
+ mTestNetwork /* expectNetwork */, MMS_APN /* expectApn */);
+ }
+
+ @Test
+ public void testAvailableNetwork_newNetworkIsSuspend() throws Exception {
+ final ArgumentCaptor<NetworkCallback> callbackCaptor =
+ ArgumentCaptor.forClass(NetworkCallback.class);
+ final CompletableFuture<String> future =
+ acquireNetwork(Integer.toString(mRequestId.getAndIncrement()));
+ verify(mCm, timeout(CALLBACK_TIMEOUT_MS).times(1))
+ .requestNetwork(any(), callbackCaptor.capture(), anyInt());
+ final NetworkCallback callback = callbackCaptor.getValue();
+
+ callback.onCapabilitiesChanged(mTestNetwork, SUSPEND_NC);
+ assertFalse(future.isDone());
+ // mNetwork should be null.
+ assertEquals(null, mMnm.getApnName());
+ verify(mCm, never()).unregisterNetworkCallback(eq(callback));
+ }
+
+ @Test
+ public void testAvailableNetwork_networkSuspendThenResume() throws Exception {
+ final NetworkCallback callback = acquireAvailableNetworkAndGetCallback(
+ mTestNetwork /* expectNetwork */, MMS_APN /* expectApn */);
+
+ // Network becomes suspended. mNetwork should be null.
+ callback.onCapabilitiesChanged(mTestNetwork, SUSPEND_NC);
+ assertEquals(null, mMnm.getApnName());
+ verify(mCm, never()).unregisterNetworkCallback(eq(callback));
+
+ // Network resume
+ callback.onCapabilitiesChanged(mTestNetwork, USABLE_NC);
+ assertEquals(MMS_APN, mMnm.getApnName());
+ }
+
+ @Test
+ public void testAvailableNetwork_networkReplaced() throws Exception {
+ final NetworkCallback callback = acquireAvailableNetworkAndGetCallback(
+ mTestNetwork /* expectNetwork */, MMS_APN /* expectApn */);
+
+ // Previous network lost. Callback will not release to wait for possible network.
+ callback.onLost(mTestNetwork);
+ verify(mCm, never()).unregisterNetworkCallback(eq(callback));
+ // New mTestNetwork2 available
+ callback.onCapabilitiesChanged(mTestNetwork2, USABLE_NC);
+ assertEquals(MMS_APN2, mMnm.getApnName());
+ }
+
+ @Test
+ public void testAvailableNetwork_networkBecomeSuspend() throws Exception {
+ final NetworkCallback callback = acquireAvailableNetworkAndGetCallback(
+ mTestNetwork /* expectNetwork */, MMS_APN /* expectApn */);
+
+ callback.onCapabilitiesChanged(mTestNetwork, SUSPEND_NC);
+ // mNetwork should be null.
+ assertEquals(null, mMnm.getApnName());
+ // Callback will not release to wait for possible network.
+ verify(mCm, never()).unregisterNetworkCallback(eq(callback));
+ }
+
+ @Test
+ public void testAvailableNetwork_networkUnavailable() throws Exception {
+ doReturn(100).when(mDeps).getNetworkRequestTimeoutMillis();
+ doReturn(100).when(mDeps).getAdditionalNetworkAcquireTimeoutMillis();
+ final ArgumentCaptor<NetworkCallback> callbackCaptor =
+ ArgumentCaptor.forClass(NetworkCallback.class);
+ final CompletableFuture<String> future =
+ acquireNetwork(Integer.toString(mRequestId.getAndIncrement()));
+ verify(mCm, timeout(CALLBACK_TIMEOUT_MS).times(1))
+ .requestNetwork(any(), callbackCaptor.capture(), anyInt());
+ final NetworkCallback callback = callbackCaptor.getValue();
+
+ assertFalse(future.isDone());
+ // No network available after 100+100 ms. Callback will be released.
+ verify(mCm, timeout(CALLBACK_TIMEOUT_MS).times(1))
+ .unregisterNetworkCallback(eq(callback));
+
+ // mNetwork should be null.
+ assertEquals(null, mMnm.getApnName());
+ }
+
+ @Test
+ public void testAvailableNetwork_newNetworkSuspendedEventuallyReleased() throws Exception {
+ doReturn(100).when(mDeps).getNetworkRequestTimeoutMillis();
+ doReturn(100).when(mDeps).getAdditionalNetworkAcquireTimeoutMillis();
+ final ArgumentCaptor<NetworkCallback> callbackCaptor =
+ ArgumentCaptor.forClass(NetworkCallback.class);
+ final CompletableFuture<String> future =
+ acquireNetwork(Integer.toString(mRequestId.getAndIncrement()));
+ verify(mCm, timeout(CALLBACK_TIMEOUT_MS).times(1))
+ .requestNetwork(any(), callbackCaptor.capture(), anyInt());
+ final NetworkCallback callback = callbackCaptor.getValue();
+
+ // New network but not a usable network.
+ callback.onCapabilitiesChanged(mTestNetwork, SUSPEND_NC);
+
+ assertFalse(future.isDone());
+ // No network available after 100+100 ms. Callback will be released.
+ verify(mCm, timeout(CALLBACK_TIMEOUT_MS).times(1))
+ .unregisterNetworkCallback(eq(callback));
+
+ // mNetwork should be null.
+ assertEquals(null, mMnm.getApnName());
+ }
+
+ private NetworkCallback acquireAvailableNetworkAndGetCallback(
+ Network expectNetwork, String expectApn) throws Exception {
+ final ArgumentCaptor<NetworkCallback> callbackCaptor =
+ ArgumentCaptor.forClass(NetworkCallback.class);
+ final CompletableFuture<String> future =
+ acquireNetwork(Integer.toString(mRequestId.getAndIncrement()));
+ verify(mCm, timeout(CALLBACK_TIMEOUT_MS).times(1))
+ .requestNetwork(any(), callbackCaptor.capture(), anyInt());
+ final NetworkCallback callback = callbackCaptor.getValue();
+
+ // Network available
+ callback.onCapabilitiesChanged(expectNetwork, USABLE_NC);
+ assertEquals(expectApn, future.get(CALLBACK_TIMEOUT_MS, TimeUnit.MILLISECONDS));
+ return callbackCaptor.getValue();
+ }
+
+ private CompletableFuture<String> acquireNetwork(String requestId) {
+ final CompletableFuture<String> future = new CompletableFuture();
+
+ mExecutor.execute(() -> {
+ try {
+ mMnm.acquireNetwork(requestId);
+ future.complete(mMnm.getApnName());
+ android.util.Log.d("MmsNetworkManagerTest", "acquireNetwork done");
+ } catch (Exception e) {
+ fail("Acquire network fail");
+ }
+ });
+
+ return future;
+ }
+}