Permalink
Browse files

ConnectivityManager: release all requests mapping to a callback.

This patch changes how callback unregistration works in order to be
consistent with undocumented use cases currently de-facto supported
by the API (although in a buggy way):
  - callback recycling: releasing then reregistering a callback again.
  - multiple request registrations with the same callback.

The second use case is not desirable but needs to be taken into account
for now for the purpose of correctly releasing NetworkRequests
registered in ConnectivityService.

In order to support request release in both use cases with minimal
amount of complexity for the time being the following changes are done:
  - request to callback unmapping is done synchronously at callback
    release time.
  - all requests associated to a callback are unmapped at callback
    release time.

This fixes the following issues:
  - a callback stops being triggered as soon as it is released.
    Otherwise when recycling the callback immediately, it is possible
    the previous request associated with it triggers it, confusing the
    app.
  - when a callback is registered multiple times, the requests are not
    leaked.
  - when a callback is registered multiple times and then released, the
    N-1 first registrations do not trigger the callback anymore.

In the future it would be desirable to enforce the intended 1:1 mapping
between callbacks and requests at registration time.

Bug: 35921499, 35955593, 20701525
Test: - added new tests in ConnectivityManagerTest to test releasing,
      recycling, and a disabled test for no multiple regristration.
      - new tests catch regression causing b/35921499, b/35955593.
Change-Id: Ia0917ac322fc049f76adb4743bc745989fed6d26
  • Loading branch information...
hugobenichigoogle committed Mar 6, 2017
1 parent 9ea45b6 commit dafed3d00818713b60ec09cfff482487a3405202
Showing with 212 additions and 40 deletions.
  1. +61 −38 core/java/android/net/ConnectivityManager.java
  2. +151 −2 tests/net/java/android/net/ConnectivityManagerTest.java
@@ -15,8 +15,6 @@
*/
package android.net;
import static com.android.internal.util.Preconditions.checkNotNull;
import android.annotation.IntDef;
import android.annotation.Nullable;
import android.annotation.SdkConstant;
@@ -50,16 +48,19 @@
import com.android.internal.telephony.ITelephony;
import com.android.internal.telephony.PhoneConstants;
import com.android.internal.util.Protocol;
import com.android.internal.util.MessageUtils;
import com.android.internal.util.Preconditions;
import com.android.internal.util.Protocol;
import libcore.net.event.NetworkEventDispatcher;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.net.InetAddress;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.List;
import java.util.Map;
/**
* Class that answers queries about the state of network connectivity. It also
@@ -1547,8 +1548,8 @@ public void stop() {
}
private PacketKeepalive(Network network, PacketKeepaliveCallback callback) {
checkNotNull(network, "network cannot be null");
checkNotNull(callback, "callback cannot be null");
Preconditions.checkNotNull(network, "network cannot be null");
Preconditions.checkNotNull(callback, "callback cannot be null");
mNetwork = network;
mCallback = callback;
HandlerThread thread = new HandlerThread(TAG);
@@ -1835,8 +1836,8 @@ public boolean isDefaultNetworkActive() {
* {@hide}
*/
public ConnectivityManager(Context context, IConnectivityManager service) {
mContext = checkNotNull(context, "missing context");
mService = checkNotNull(service, "missing IConnectivityManager");
mContext = Preconditions.checkNotNull(context, "missing context");
mService = Preconditions.checkNotNull(service, "missing IConnectivityManager");
sInstance = this;
}
@@ -2099,7 +2100,7 @@ public void startTethering(int type, boolean showProvisioningUi,
@SystemApi
public void startTethering(int type, boolean showProvisioningUi,
final OnStartTetheringCallback callback, Handler handler) {
checkNotNull(callback, "OnStartTetheringCallback cannot be null.");
Preconditions.checkNotNull(callback, "OnStartTetheringCallback cannot be null.");
ResultReceiver wrappedCallback = new ResultReceiver(handler) {
@Override
@@ -2559,8 +2560,16 @@ public int registerNetworkAgent(Messenger messenger, NetworkInfo ni, LinkPropert
}
/**
* Base class for NetworkRequest callbacks. Used for notifications about network
* changes. Should be extended by applications wanting notifications.
* Base class for {@code NetworkRequest} callbacks. Used for notifications about network
* changes. Should be extended by applications wanting notifications.
*
* A {@code NetworkCallback} is registered by calling
* {@link #requestNetwork(NetworkRequest, NetworkCallback)},
* {@link #registerNetworkCallback(NetworkRequest, NetworkCallback)},
* or {@link #registerDefaultNetworkCallback(NetworkCallback). A {@code NetworkCallback} is
* unregistered by calling {@link #unregisterNetworkCallback(NetworkCallback)}.
* A {@code NetworkCallback} should be registered at most once at any time.
* A {@code NetworkCallback} that has been unregistered can be registered again.
*/
public static class NetworkCallback {
/**
@@ -2663,6 +2672,10 @@ public void onNetworkSuspended(Network network) {}
public void onNetworkResumed(Network network) {}
private NetworkRequest networkRequest;
private boolean isRegistered() {
return (networkRequest != null) && (networkRequest.requestId != REQUEST_ID_UNSET);
}
}
private static final int BASE = Protocol.BASE_CONNECTIVITY_MANAGER;
@@ -2680,6 +2693,7 @@ public void onNetworkResumed(Network network) {}
public static final int CALLBACK_CAP_CHANGED = BASE + 6;
/** @hide */
public static final int CALLBACK_IP_CHANGED = BASE + 7;
// TODO: consider deleting CALLBACK_RELEASED and shifting following enum codes down by 1.
/** @hide */
public static final int CALLBACK_RELEASED = BASE + 8;
// TODO: consider deleting CALLBACK_EXIT and shifting following enum codes down by 1.
@@ -2798,13 +2812,6 @@ public void handleMessage(Message message) {
break;
}
case CALLBACK_RELEASED: {
final NetworkCallback callback;
synchronized(sCallbacks) {
callback = sCallbacks.remove(request);
}
if (callback == null) {
Log.e(TAG, "callback not found for RELEASED message");
}
break;
}
case CALLBACK_EXIT: {
@@ -2822,12 +2829,12 @@ public void handleMessage(Message message) {
}
private NetworkCallback getCallback(NetworkRequest req, String name) {
NetworkCallback callback;
final NetworkCallback callback;
synchronized(sCallbacks) {
callback = sCallbacks.get(req);
}
if (callback == null) {
Log.e(TAG, "callback not found for " + name + " message");
Log.w(TAG, "callback not found for " + name + " message");
}
return callback;
}
@@ -2850,17 +2857,16 @@ private CallbackHandler getDefaultHandler() {
private NetworkRequest sendRequestForNetwork(NetworkCapabilities need, NetworkCallback callback,
int timeoutMs, int action, int legacyType, CallbackHandler handler) {
if (callback == null) {
throw new IllegalArgumentException("null NetworkCallback");
}
if (need == null && action != REQUEST) {
throw new IllegalArgumentException("null NetworkCapabilities");
}
// TODO: throw an exception if callback.networkRequest is not null.
// http://b/20701525
Preconditions.checkArgument(callback != null, "null NetworkCallback");
Preconditions.checkArgument(action == REQUEST || need != null, "null NetworkCapabilities");
final NetworkRequest request;
try {
synchronized(sCallbacks) {
if (callback.isRegistered()) {
// TODO: throw exception instead and enforce 1:1 mapping of callbacks
// and requests (http://b/20701525).
Log.e(TAG, "NetworkCallback was already registered");
}
Messenger messenger = new Messenger(handler);
Binder binder = new Binder();
if (action == LISTEN) {
@@ -3325,25 +3331,42 @@ public boolean requestBandwidthUpdate(Network network) {
}
/**
* Unregisters callbacks about and possibly releases networks originating from
* Unregisters a {@code NetworkCallback} and possibly releases networks originating from
* {@link #requestNetwork(NetworkRequest, NetworkCallback)} and
* {@link #registerNetworkCallback(NetworkRequest, NetworkCallback)} calls.
* If the given {@code NetworkCallback} had previously been used with
* {@code #requestNetwork}, any networks that had been connected to only to satisfy that request
* will be disconnected.
*
* Notifications that would have triggered that {@code NetworkCallback} will immediately stop
* triggering it as soon as this call returns.
*
* @param networkCallback The {@link NetworkCallback} used when making the request.
*/
public void unregisterNetworkCallback(NetworkCallback networkCallback) {
if (networkCallback == null || networkCallback.networkRequest == null ||
networkCallback.networkRequest.requestId == REQUEST_ID_UNSET) {
throw new IllegalArgumentException("Invalid NetworkCallback");
}
try {
// CallbackHandler will release callback when receiving CALLBACK_RELEASED.
mService.releaseNetworkRequest(networkCallback.networkRequest);
} catch (RemoteException e) {
throw e.rethrowFromSystemServer();
Preconditions.checkArgument(networkCallback != null, "null NetworkCallback");
final List<NetworkRequest> reqs = new ArrayList<>();
// Find all requests associated to this callback and stop callback triggers immediately.
// Callback is reusable immediately. http://b/20701525, http://b/35921499.
synchronized (sCallbacks) {
Preconditions.checkArgument(
networkCallback.isRegistered(), "NetworkCallback was not registered");
for (Map.Entry<NetworkRequest, NetworkCallback> e : sCallbacks.entrySet()) {
if (e.getValue() == networkCallback) {
reqs.add(e.getKey());
}
}
// TODO: throw exception if callback was registered more than once (http://b/20701525).
for (NetworkRequest r : reqs) {
try {
mService.releaseNetworkRequest(r);
} catch (RemoteException e) {
throw e.rethrowFromSystemServer();
}
// Only remove mapping if rpc was successful.
sCallbacks.remove(r);
}
networkCallback.networkRequest = null;
}
}
@@ -36,21 +36,50 @@
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.anyInt;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.timeout;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import android.net.ConnectivityManager;
import android.net.NetworkCapabilities;
import android.content.Context;
import android.os.Bundle;
import android.os.Handler;
import android.os.Looper;
import android.os.Message;
import android.os.Messenger;
import android.content.pm.ApplicationInfo;
import android.os.Build.VERSION_CODES;
import android.net.ConnectivityManager.NetworkCallback;
import android.support.test.filters.SmallTest;
import android.support.test.runner.AndroidJUnit4;
import org.junit.runner.RunWith;
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;
@RunWith(AndroidJUnit4.class)
@SmallTest
public class ConnectivityManagerTest {
@Mock Context mCtx;
@Mock IConnectivityManager mService;
@Before
public void setUp() {
MockitoAnnotations.initMocks(this);
}
static NetworkCapabilities verifyNetworkCapabilities(
int legacyType, int transportType, int... capabilities) {
final NetworkCapabilities nc = ConnectivityManager.networkCapabilitiesForType(legacyType);
@@ -173,4 +202,124 @@ public void testNetworkCapabilitiesForTypeEthernet() {
verifyUnrestrictedNetworkCapabilities(
ConnectivityManager.TYPE_ETHERNET, TRANSPORT_ETHERNET);
}
@Test
public void testCallbackRelease() throws Exception {
ConnectivityManager manager = new ConnectivityManager(mCtx, mService);
NetworkRequest request = makeRequest(1);
NetworkCallback callback = mock(ConnectivityManager.NetworkCallback.class);
Handler handler = new Handler(Looper.getMainLooper());
ArgumentCaptor<Messenger> captor = ArgumentCaptor.forClass(Messenger.class);
// register callback
when(mService.requestNetwork(any(), captor.capture(), anyInt(), any(), anyInt()))
.thenReturn(request);
manager.requestNetwork(request, callback, handler);
// callback triggers
captor.getValue().send(makeMessage(request, ConnectivityManager.CALLBACK_AVAILABLE));
verify(callback, timeout(500).times(1)).onAvailable(any());
// unregister callback
manager.unregisterNetworkCallback(callback);
verify(mService, times(1)).releaseNetworkRequest(request);
// callback does not trigger anymore.
captor.getValue().send(makeMessage(request, ConnectivityManager.CALLBACK_LOSING));
verify(callback, timeout(500).times(0)).onLosing(any(), anyInt());
}
@Test
public void testCallbackRecycling() throws Exception {
ConnectivityManager manager = new ConnectivityManager(mCtx, mService);
NetworkRequest req1 = makeRequest(1);
NetworkRequest req2 = makeRequest(2);
NetworkCallback callback = mock(ConnectivityManager.NetworkCallback.class);
Handler handler = new Handler(Looper.getMainLooper());
ArgumentCaptor<Messenger> captor = ArgumentCaptor.forClass(Messenger.class);
// register callback
when(mService.requestNetwork(any(), captor.capture(), anyInt(), any(), anyInt()))
.thenReturn(req1);
manager.requestNetwork(req1, callback, handler);
// callback triggers
captor.getValue().send(makeMessage(req1, ConnectivityManager.CALLBACK_AVAILABLE));
verify(callback, timeout(100).times(1)).onAvailable(any());
// unregister callback
manager.unregisterNetworkCallback(callback);
verify(mService, times(1)).releaseNetworkRequest(req1);
// callback does not trigger anymore.
captor.getValue().send(makeMessage(req1, ConnectivityManager.CALLBACK_LOSING));
verify(callback, timeout(100).times(0)).onLosing(any(), anyInt());
// callback can be registered again
when(mService.requestNetwork(any(), captor.capture(), anyInt(), any(), anyInt()))
.thenReturn(req2);
manager.requestNetwork(req2, callback, handler);
// callback triggers
captor.getValue().send(makeMessage(req2, ConnectivityManager.CALLBACK_LOST));
verify(callback, timeout(100).times(1)).onLost(any());
// unregister callback
manager.unregisterNetworkCallback(callback);
verify(mService, times(1)).releaseNetworkRequest(req2);
}
// TODO: turn on this test when request callback 1:1 mapping is enforced
//@Test
private void noDoubleCallbackRegistration() throws Exception {
ConnectivityManager manager = new ConnectivityManager(mCtx, mService);
NetworkRequest request = makeRequest(1);
NetworkCallback callback = new ConnectivityManager.NetworkCallback();
ApplicationInfo info = new ApplicationInfo();
// TODO: update version when starting to enforce 1:1 mapping
info.targetSdkVersion = VERSION_CODES.N_MR1 + 1;
when(mCtx.getApplicationInfo()).thenReturn(info);
when(mService.requestNetwork(any(), any(), anyInt(), any(), anyInt())).thenReturn(request);
Handler handler = new Handler(Looper.getMainLooper());
manager.requestNetwork(request, callback, handler);
// callback is already registered, reregistration should fail.
Class<IllegalArgumentException> wantException = IllegalArgumentException.class;
expectThrowable(() -> manager.requestNetwork(request, callback), wantException);
manager.unregisterNetworkCallback(callback);
verify(mService, times(1)).releaseNetworkRequest(request);
// unregistering the callback should make it registrable again.
manager.requestNetwork(request, callback);
}
static Message makeMessage(NetworkRequest req, int messageType) {
Bundle bundle = new Bundle();
bundle.putParcelable(NetworkRequest.class.getSimpleName(), req);
Message msg = Message.obtain();
msg.what = messageType;
msg.setData(bundle);
return msg;
}
static NetworkRequest makeRequest(int requestId) {
NetworkRequest request = new NetworkRequest.Builder().clearCapabilities().build();
return new NetworkRequest(request.networkCapabilities, ConnectivityManager.TYPE_NONE,
requestId, NetworkRequest.Type.NONE);
}
static void expectThrowable(Runnable block, Class<? extends Throwable> throwableType) {
try {
block.run();
} catch (Throwable t) {
if (t.getClass().equals(throwableType)) {
return;
}
fail("expected exception of type " + throwableType + ", but was " + t.getClass());
}
fail("expected exception of type " + throwableType);
}
}

0 comments on commit dafed3d

Please sign in to comment.