From f2c64f87259679c44ef151b08cccab43ca5d4342 Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Tue, 2 May 2017 13:36:28 +0900 Subject: [PATCH 1/3] NsdManager: remove duplicated argument validation This patch simplifies argument validation in NsdManager public api and regroup duplicated validation into common methods. This makes stack traces more actionable as now specific errors will cause the api to throw exception from specific methods, whereas before IllegalArgumentException would be thrown from inside the same api method for different reasons. This patch also includes a couple of other small cleanups. Test: $ runtest -x frameworks/base/tests/net/../NsdManagerTest.java Bug: 37013369 Change-Id: Iaad13e13976e9bf8f508d7188f823f8184ac414b --- core/java/android/net/nsd/NsdManager.java | 176 +++++++++------------- 1 file changed, 73 insertions(+), 103 deletions(-) diff --git a/core/java/android/net/nsd/NsdManager.java b/core/java/android/net/nsd/NsdManager.java index 3fd9f19364..13648d2c76 100644 --- a/core/java/android/net/nsd/NsdManager.java +++ b/core/java/android/net/nsd/NsdManager.java @@ -16,6 +16,10 @@ package android.net.nsd; +import static com.android.internal.util.Preconditions.checkArgument; +import static com.android.internal.util.Preconditions.checkNotNull; +import static com.android.internal.util.Preconditions.checkStringNotEmpty; + import android.annotation.SdkConstant; import android.annotation.SdkConstant.SdkConstantType; import android.content.Context; @@ -241,12 +245,12 @@ public final class NsdManager { return name; } + private static int FIRST_LISTENER_KEY = 1; + private final INsdManager mService; private final Context mContext; - private static final int INVALID_LISTENER_KEY = 0; - private static final int BUSY_LISTENER_KEY = -1; - private int mListenerKey = 1; + private int mListenerKey = FIRST_LISTENER_KEY; private final SparseArray mListenerMap = new SparseArray(); private final SparseArray mServiceMap = new SparseArray<>(); private final Object mMapLock = new Object(); @@ -304,7 +308,6 @@ public final class NsdManager { public void onServiceFound(NsdServiceInfo serviceInfo); public void onServiceLost(NsdServiceInfo serviceInfo); - } /** Interface for callback invocation for service registration */ @@ -335,8 +338,9 @@ public final class NsdManager { @Override public void handleMessage(Message message) { - if (DBG) Log.d(TAG, "received " + nameOf(message.what)); - switch (message.what) { + final int what = message.what; + final int key = message.arg2; + switch (what) { case AsyncChannel.CMD_CHANNEL_HALF_CONNECTED: mAsyncChannel.sendMessage(AsyncChannel.CMD_CHANNEL_FULL_CONNECTION); return; @@ -349,19 +353,26 @@ public final class NsdManager { default: break; } - Object listener = getListener(message.arg2); + final Object listener; + final NsdServiceInfo ns; + synchronized (mMapLock) { + listener = mListenerMap.get(key); + ns = mServiceMap.get(key); + } if (listener == null) { Log.d(TAG, "Stale key " + message.arg2); return; } - NsdServiceInfo ns = getNsdService(message.arg2); - switch (message.what) { + if (DBG) { + Log.d(TAG, "received " + nameOf(what) + " for key " + key + ", service " + ns); + } + switch (what) { case DISCOVER_SERVICES_STARTED: String s = getNsdServiceInfoType((NsdServiceInfo) message.obj); ((DiscoveryListener) listener).onDiscoveryStarted(s); break; case DISCOVER_SERVICES_FAILED: - removeListener(message.arg2); + removeListener(key); ((DiscoveryListener) listener).onStartDiscoveryFailed(getNsdServiceInfoType(ns), message.arg1); break; @@ -374,16 +385,16 @@ public final class NsdManager { case STOP_DISCOVERY_FAILED: // TODO: failure to stop discovery should be internal and retried internally, as // the effect for the client is indistinguishable from STOP_DISCOVERY_SUCCEEDED - removeListener(message.arg2); + removeListener(key); ((DiscoveryListener) listener).onStopDiscoveryFailed(getNsdServiceInfoType(ns), message.arg1); break; case STOP_DISCOVERY_SUCCEEDED: - removeListener(message.arg2); + removeListener(key); ((DiscoveryListener) listener).onDiscoveryStopped(getNsdServiceInfoType(ns)); break; case REGISTER_SERVICE_FAILED: - removeListener(message.arg2); + removeListener(key); ((RegistrationListener) listener).onRegistrationFailed(ns, message.arg1); break; case REGISTER_SERVICE_SUCCEEDED: @@ -391,7 +402,7 @@ public final class NsdManager { (NsdServiceInfo) message.obj); break; case UNREGISTER_SERVICE_FAILED: - removeListener(message.arg2); + removeListener(key); ((RegistrationListener) listener).onUnregistrationFailed(ns, message.arg1); break; case UNREGISTER_SERVICE_SUCCEEDED: @@ -401,11 +412,11 @@ public final class NsdManager { ((RegistrationListener) listener).onServiceUnregistered(ns); break; case RESOLVE_SERVICE_FAILED: - removeListener(message.arg2); + removeListener(key); ((ResolveListener) listener).onResolveFailed(ns, message.arg1); break; case RESOLVE_SERVICE_SUCCEEDED: - removeListener(message.arg2); + removeListener(key); ((ResolveListener) listener).onServiceResolved((NsdServiceInfo) message.obj); break; default: @@ -415,40 +426,27 @@ public final class NsdManager { } } - // if the listener is already in the map, reject it. Otherwise, add it and - // return its key. + private int nextListenerKey() { + // Ensure mListenerKey >= FIRST_LISTENER_KEY; + mListenerKey = Math.max(FIRST_LISTENER_KEY, mListenerKey + 1); + return mListenerKey; + } + + // Assert that the listener is not in the map, then add it and returns its key private int putListener(Object listener, NsdServiceInfo s) { - if (listener == null) return INVALID_LISTENER_KEY; - int key; + checkListener(listener); + final int key; synchronized (mMapLock) { int valueIndex = mListenerMap.indexOfValue(listener); - if (valueIndex != -1) { - return BUSY_LISTENER_KEY; - } - do { - key = mListenerKey++; - } while (key == INVALID_LISTENER_KEY); + checkArgument(valueIndex == -1, "listener already in use"); + key = nextListenerKey(); mListenerMap.put(key, listener); mServiceMap.put(key, s); } return key; } - private Object getListener(int key) { - if (key == INVALID_LISTENER_KEY) return null; - synchronized (mMapLock) { - return mListenerMap.get(key); - } - } - - private NsdServiceInfo getNsdService(int key) { - synchronized (mMapLock) { - return mServiceMap.get(key); - } - } - private void removeListener(int key) { - if (key == INVALID_LISTENER_KEY) return; synchronized (mMapLock) { mListenerMap.remove(key); mServiceMap.remove(key); @@ -456,16 +454,15 @@ public final class NsdManager { } private int getListenerKey(Object listener) { + checkListener(listener); synchronized (mMapLock) { int valueIndex = mListenerMap.indexOfValue(listener); - if (valueIndex != -1) { - return mListenerMap.keyAt(valueIndex); - } + checkArgument(valueIndex != -1, "listener not registered"); + return mListenerMap.keyAt(valueIndex); } - return INVALID_LISTENER_KEY; } - private String getNsdServiceInfoType(NsdServiceInfo s) { + private static String getNsdServiceInfoType(NsdServiceInfo s) { if (s == null) return "?"; return s.getServiceType(); } @@ -475,7 +472,9 @@ public final class NsdManager { */ private void init() { final Messenger messenger = getMessenger(); - if (messenger == null) throw new RuntimeException("Failed to initialize"); + if (messenger == null) { + fatal("Failed to obtain service Messenger"); + } HandlerThread t = new HandlerThread("NsdManager"); t.start(); mHandler = new ServiceHandler(t.getLooper()); @@ -483,10 +482,15 @@ public final class NsdManager { try { mConnected.await(); } catch (InterruptedException e) { - Log.e(TAG, "interrupted wait at init"); + fatal("Interrupted wait at init"); } } + private static void fatal(String msg) { + Log.e(TAG, msg); + throw new RuntimeException(msg); + } + /** * Register a service to be discovered by other services. * @@ -506,23 +510,10 @@ public final class NsdManager { */ public void registerService(NsdServiceInfo serviceInfo, int protocolType, RegistrationListener listener) { - if (TextUtils.isEmpty(serviceInfo.getServiceName()) || - TextUtils.isEmpty(serviceInfo.getServiceType())) { - throw new IllegalArgumentException("Service name or type cannot be empty"); - } - if (serviceInfo.getPort() <= 0) { - throw new IllegalArgumentException("Invalid port number"); - } - if (listener == null) { - throw new IllegalArgumentException("listener cannot be null"); - } - if (protocolType != PROTOCOL_DNS_SD) { - throw new IllegalArgumentException("Unsupported protocol"); - } + checkArgument(serviceInfo.getPort() > 0, "Invalid port number"); + checkServiceInfo(serviceInfo); + checkProtocol(protocolType); int key = putListener(listener, serviceInfo); - if (key == BUSY_LISTENER_KEY) { - throw new IllegalArgumentException("listener already in use"); - } mAsyncChannel.sendMessage(REGISTER_SERVICE, 0, key, serviceInfo); } @@ -541,12 +532,6 @@ public final class NsdManager { */ public void unregisterService(RegistrationListener listener) { int id = getListenerKey(listener); - if (id == INVALID_LISTENER_KEY) { - throw new IllegalArgumentException("listener not registered"); - } - if (listener == null) { - throw new IllegalArgumentException("listener cannot be null"); - } mAsyncChannel.sendMessage(UNREGISTER_SERVICE, 0, id); } @@ -579,25 +564,13 @@ public final class NsdManager { * Cannot be null. Cannot be in use for an active service discovery. */ public void discoverServices(String serviceType, int protocolType, DiscoveryListener listener) { - if (listener == null) { - throw new IllegalArgumentException("listener cannot be null"); - } - if (TextUtils.isEmpty(serviceType)) { - throw new IllegalArgumentException("Service type cannot be empty"); - } - - if (protocolType != PROTOCOL_DNS_SD) { - throw new IllegalArgumentException("Unsupported protocol"); - } + checkStringNotEmpty(serviceType, "Service type cannot be empty"); + checkProtocol(protocolType); NsdServiceInfo s = new NsdServiceInfo(); s.setServiceType(serviceType); int key = putListener(listener, s); - if (key == BUSY_LISTENER_KEY) { - throw new IllegalArgumentException("listener already in use"); - } - mAsyncChannel.sendMessage(DISCOVER_SERVICES, 0, key, s); } @@ -619,12 +592,6 @@ public final class NsdManager { */ public void stopServiceDiscovery(DiscoveryListener listener) { int id = getListenerKey(listener); - if (id == INVALID_LISTENER_KEY) { - throw new IllegalArgumentException("service discovery not active on listener"); - } - if (listener == null) { - throw new IllegalArgumentException("listener cannot be null"); - } mAsyncChannel.sendMessage(STOP_DISCOVERY, 0, id); } @@ -638,19 +605,8 @@ public final class NsdManager { * Cannot be in use for an active service resolution. */ public void resolveService(NsdServiceInfo serviceInfo, ResolveListener listener) { - if (TextUtils.isEmpty(serviceInfo.getServiceName()) || - TextUtils.isEmpty(serviceInfo.getServiceType())) { - throw new IllegalArgumentException("Service name or type cannot be empty"); - } - if (listener == null) { - throw new IllegalArgumentException("listener cannot be null"); - } - + checkServiceInfo(serviceInfo); int key = putListener(listener, serviceInfo); - - if (key == BUSY_LISTENER_KEY) { - throw new IllegalArgumentException("listener already in use"); - } mAsyncChannel.sendMessage(RESOLVE_SERVICE, 0, key, serviceInfo); } @@ -664,10 +620,10 @@ public final class NsdManager { } /** - * Get a reference to NetworkService handler. This is used to establish + * Get a reference to NsdService handler. This is used to establish * an AsyncChannel communication with the service * - * @return Messenger pointing to the NetworkService handler + * @return Messenger pointing to the NsdService handler */ private Messenger getMessenger() { try { @@ -676,4 +632,18 @@ public final class NsdManager { throw e.rethrowFromSystemServer(); } } + + private static void checkListener(Object listener) { + checkNotNull(listener, "listener cannot be null"); + } + + private static void checkProtocol(int protocolType) { + checkArgument(protocolType == PROTOCOL_DNS_SD, "Unsupported protocol"); + } + + private static void checkServiceInfo(NsdServiceInfo serviceInfo) { + checkNotNull(serviceInfo, "NsdServiceInfo cannot be null"); + checkStringNotEmpty(serviceInfo.getServiceName(),"Service name cannot be empty"); + checkStringNotEmpty(serviceInfo.getServiceType(), "Service type cannot be empty"); + } } From 4dd4db77209ab2ef302a3a2a4892b05aadd26ec6 Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Fri, 28 Apr 2017 15:31:10 +0900 Subject: [PATCH 2/3] NsdService: test coverage for client requests. Adding coverage for: - NsdManager client disconnection - in-flight request GC Test: new test passes Bug: 37013369, 33298084 Change-Id: I92039f297cf99352bbf4196797933d89c0b819ff --- core/java/android/net/nsd/NsdManager.java | 8 +++++ .../java/com/android/server/NsdService.java | 35 ++++++------------- 2 files changed, 18 insertions(+), 25 deletions(-) diff --git a/core/java/android/net/nsd/NsdManager.java b/core/java/android/net/nsd/NsdManager.java index 13648d2c76..27ca6e2e51 100644 --- a/core/java/android/net/nsd/NsdManager.java +++ b/core/java/android/net/nsd/NsdManager.java @@ -273,6 +273,14 @@ public final class NsdManager { init(); } + /** + * @hide + */ + @VisibleForTesting + public void disconnect() { + mAsyncChannel.disconnect(); + } + /** * Failures are passed with {@link RegistrationListener#onRegistrationFailed}, * {@link RegistrationListener#onUnregistrationFailed}, diff --git a/services/core/java/com/android/server/NsdService.java b/services/core/java/com/android/server/NsdService.java index 55bca648d5..efbad450af 100644 --- a/services/core/java/com/android/server/NsdService.java +++ b/services/core/java/com/android/server/NsdService.java @@ -48,7 +48,6 @@ import com.android.internal.util.AsyncChannel; import com.android.internal.util.Protocol; import com.android.internal.util.State; import com.android.internal.util.StateMachine; -import com.android.server.NativeDaemonConnector.Command; /** * Network Service Discovery Service handles remote service discovery operation requests by @@ -161,7 +160,7 @@ public class NsdService extends INsdManager.Stub { } //Last client if (mClients.size() == 0) { - stopMDnsDaemon(); + mDaemon.stop(); } break; case AsyncChannel.CMD_CHANNEL_FULL_CONNECTION: @@ -221,14 +220,14 @@ public class NsdService extends INsdManager.Stub { public void enter() { sendNsdStateChangeBroadcast(true); if (mClients.size() > 0) { - startMDnsDaemon(); + mDaemon.start(); } } @Override public void exit() { if (mClients.size() > 0) { - stopMDnsDaemon(); + mDaemon.stop(); } } @@ -262,7 +261,7 @@ public class NsdService extends INsdManager.Stub { //First client if (msg.arg1 == AsyncChannel.STATUS_SUCCESSFUL && mClients.size() == 0) { - startMDnsDaemon(); + mDaemon.start(); } return NOT_HANDLED; case AsyncChannel.CMD_CHANNEL_DISCONNECTED: @@ -712,26 +711,13 @@ public class NsdService extends INsdManager.Stub { return true; } - public boolean execute(Command cmd) { - if (DBG) { - Slog.d(TAG, cmd.toString()); - } - try { - mNativeConnector.execute(cmd); - } catch (NativeDaemonConnectorException e) { - Slog.e(TAG, "Failed to execute " + cmd, e); - return false; - } - return true; + public void start() { + execute("start-service"); } - } - private boolean startMDnsDaemon() { - return mDaemon.execute("start-service"); - } - - private boolean stopMDnsDaemon() { - return mDaemon.execute("stop-service"); + public void stop() { + execute("stop-service"); + } } private boolean registerService(int regId, NsdServiceInfo service) { @@ -743,8 +729,7 @@ public class NsdService extends INsdManager.Stub { int port = service.getPort(); byte[] textRecord = service.getTxtRecord(); String record = Base64.encodeToString(textRecord, Base64.DEFAULT).replace("\n", ""); - Command cmd = new Command("mdnssd", "register", regId, name, type, port, record); - return mDaemon.execute(cmd); + return mDaemon.execute("register", regId, name, type, port, record); } private boolean unregisterService(int regId) { From d2552aee119299bf66d694960c72a00141642ef5 Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Tue, 11 Apr 2017 14:42:47 +0900 Subject: [PATCH 3/3] NsdService: simple cleanups This patch replace some SparseArray with SparseIntArray, and simplify a value lookup function by using indexOfValue(). Test: TODO Bug: 37013369, 33298084 Change-Id: I4872f8baa2bb2ff456c7f848d3afe2e7bcc9892e --- .../java/com/android/server/NsdService.java | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/services/core/java/com/android/server/NsdService.java b/services/core/java/com/android/server/NsdService.java index efbad450af..965835c4b2 100644 --- a/services/core/java/com/android/server/NsdService.java +++ b/services/core/java/com/android/server/NsdService.java @@ -35,6 +35,7 @@ import android.provider.Settings; import android.util.Base64; import android.util.Slog; import android.util.SparseArray; +import android.util.SparseIntArray; import java.io.FileDescriptor; import java.io.PrintWriter; @@ -246,8 +247,8 @@ public class NsdService extends INsdManager.Stub { } private void removeRequestMap(int clientId, int globalId, ClientInfo clientInfo) { - clientInfo.mClientIds.remove(clientId); - clientInfo.mClientRequests.remove(clientId); + clientInfo.mClientIds.delete(clientId); + clientInfo.mClientRequests.delete(clientId); mIdToClientInfoMap.remove(globalId); } @@ -300,7 +301,7 @@ public class NsdService extends INsdManager.Stub { clientInfo = mClients.get(msg.replyTo); try { - id = clientInfo.mClientIds.get(msg.arg2).intValue(); + id = clientInfo.mClientIds.get(msg.arg2); } catch (NullPointerException e) { replyToMessage(msg, NsdManager.STOP_DISCOVERY_FAILED, NsdManager.FAILURE_INTERNAL_ERROR); @@ -338,7 +339,7 @@ public class NsdService extends INsdManager.Stub { if (DBG) Slog.d(TAG, "unregister service"); clientInfo = mClients.get(msg.replyTo); try { - id = clientInfo.mClientIds.get(msg.arg2).intValue(); + id = clientInfo.mClientIds.get(msg.arg2); } catch (NullPointerException e) { replyToMessage(msg, NsdManager.UNREGISTER_SERVICE_FAILED, NsdManager.FAILURE_INTERNAL_ERROR); @@ -828,10 +829,10 @@ public class NsdService extends INsdManager.Stub { private NsdServiceInfo mResolvedService; /* A map from client id to unique id sent to mDns */ - private final SparseArray mClientIds = new SparseArray<>(); + private final SparseIntArray mClientIds = new SparseIntArray(); /* A map from client id to the type of the request we had received */ - private final SparseArray mClientRequests = new SparseArray<>(); + private final SparseIntArray mClientRequests = new SparseIntArray(); private ClientInfo(AsyncChannel c, Messenger m) { mChannel = c; @@ -858,6 +859,7 @@ public class NsdService extends INsdManager.Stub { // and send cancellations to the daemon. private void expungeAllRequests() { int globalId, clientId, i; + // TODO: to keep handler responsive, do not clean all requests for that client at once. for (i = 0; i < mClientIds.size(); i++) { clientId = mClientIds.keyAt(i); globalId = mClientIds.valueAt(i); @@ -885,15 +887,11 @@ public class NsdService extends INsdManager.Stub { // mClientIds is a sparse array of listener id -> mDnsClient id. For a given mDnsClient id, // return the corresponding listener id. mDnsClient id is also called a global id. private int getClientId(final int globalId) { - // This doesn't use mClientIds.indexOfValue because indexOfValue uses == (not .equals) - // while also coercing the int primitives to Integer objects. - for (int i = 0, nSize = mClientIds.size(); i < nSize; i++) { - int mDnsId = mClientIds.valueAt(i); - if (globalId == mDnsId) { - return mClientIds.keyAt(i); - } + int idx = mClientIds.indexOfValue(globalId); + if (idx < 0) { + return idx; } - return -1; + return mClientIds.keyAt(idx); } }