From c93888f5d7dcb2e21d462e79727ec562ddd9fb87 Mon Sep 17 00:00:00 2001 From: Chiachang Wang Date: Thu, 24 Jun 2021 22:56:21 +0800 Subject: Fix the logic for handling network status change In the current design, MmsNetworkManager assume that there is only one network will be available for mms request. It releases the request when the network lost. But there may be multiple networks if the carrier network supports it. Another issue is that a suspended network should not be an available network. When the network becomes suspended, the target network object should be clear to allow callers to know that there is no available network until the network resumes. Starting from O, onAvailable will always immediately be followed by a onCapabilitiesChanged. The available network may also be a suspended network. It will need to check if the coming available network is usable or not. Thus, remove the onAvailable callback to check the network in onCapabilitiesChanged. Bug: 172050345 Test: atest MmsNetworkManagerTest Test: manually send and receive mms Change-Id: I176477263314fac8f8be8debb16958b27933fb24 --- src/com/android/mms/service/MmsNetworkManager.java | 91 +++++--- .../android/mms/service/MmsNetworkManagerTest.java | 235 +++++++++++++++++++++ 2 files changed, 300 insertions(+), 26 deletions(-) create mode 100644 tests/robotests/src/com/android/mms/service/MmsNetworkManagerTest.java 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.provider.DeviceConfig; 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 @@ public class MmsNetworkManager { // 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 @@ -141,23 +143,13 @@ public class MmsNetworkManager { * Network callback for our network request */ 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 @@ public class MmsNetworkManager { 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 class MmsNetworkManager { }; } + public MmsNetworkManager(Context context, int subId) { + this(context, subId, new Dependencies()); + } + /** * Acquire the MMS network * @@ -207,7 +254,7 @@ public class MmsNetworkManager { * @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 @@ public class MmsNetworkManager { } // 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 @@ public class MmsNetworkManager { 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 @@ public class MmsNetworkManager { } } - // 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 callbackCaptor = + ArgumentCaptor.forClass(NetworkCallback.class); + final CompletableFuture 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 callbackCaptor = + ArgumentCaptor.forClass(NetworkCallback.class); + final CompletableFuture 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 callbackCaptor = + ArgumentCaptor.forClass(NetworkCallback.class); + final CompletableFuture 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 callbackCaptor = + ArgumentCaptor.forClass(NetworkCallback.class); + final CompletableFuture 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 acquireNetwork(String requestId) { + final CompletableFuture 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; + } +} -- cgit v1.2.3