From f2a51ac5f8cc6b1cfaaa0f8fd5c28af522806ca0 Mon Sep 17 00:00:00 2001 From: Paul Hu Date: Wed, 26 Apr 2023 10:17:48 +0800 Subject: [PATCH] Remove the ServiceTypeClient after socket destroyed The ServiceTypeClient should be removed after socket destroyed because it's no longer used by any request. The requests will be matched to the newly created ServiceTypeClient. Bug: 278635632 Test: atest FrameworksNetTests CtsNetTestCases Change-Id: Ia917b14d3666f3bfe8e874606a34800a4ce65c5a --- .../src/android/net/nsd/NsdManager.java | 15 +++++ .../mdns/MdnsDiscoveryManager.java | 42 ++++++++++---- .../mdns/MdnsMultinetworkSocketClient.java | 1 + .../mdns/MdnsServiceTypeClient.java | 22 ++++++++ .../mdns/MdnsSocketClientBase.java | 3 + .../mdns/MdnsDiscoveryManagerTests.java | 55 +++++++++++++++++++ .../mdns/MdnsServiceTypeClientTests.java | 55 +++++++++++++++++++ 7 files changed, 181 insertions(+), 12 deletions(-) diff --git a/framework-t/src/android/net/nsd/NsdManager.java b/framework-t/src/android/net/nsd/NsdManager.java index d119db6f1a..2930cbdf3c 100644 --- a/framework-t/src/android/net/nsd/NsdManager.java +++ b/framework-t/src/android/net/nsd/NsdManager.java @@ -429,6 +429,10 @@ public final class NsdManager { private final DiscoveryListener mWrapped; private final Executor mWrappedExecutor; private final ArraySet mFoundInfo = new ArraySet<>(); + // When this flag is set to true, no further service found or lost callbacks should be + // handled. This flag indicates that the network for this DelegatingDiscoveryListener is + // lost, and any further callbacks would be redundant. + private boolean mAllServicesLost = false; private DelegatingDiscoveryListener(Network network, DiscoveryListener listener, Executor executor) { @@ -445,6 +449,7 @@ public final class NsdManager { serviceInfo.setNetwork(mNetwork); mWrappedExecutor.execute(() -> mWrapped.onServiceLost(serviceInfo)); } + mAllServicesLost = true; } @Override @@ -486,12 +491,22 @@ public final class NsdManager { @Override public void onServiceFound(NsdServiceInfo serviceInfo) { + if (mAllServicesLost) { + // This DelegatingDiscoveryListener no longer has a network connection. Ignore + // the callback. + return; + } mFoundInfo.add(new TrackedNsdInfo(serviceInfo)); mWrappedExecutor.execute(() -> mWrapped.onServiceFound(serviceInfo)); } @Override public void onServiceLost(NsdServiceInfo serviceInfo) { + if (mAllServicesLost) { + // This DelegatingDiscoveryListener no longer has a network connection. Ignore + // the callback. + return; + } mFoundInfo.remove(new TrackedNsdInfo(serviceInfo)); mWrappedExecutor.execute(() -> mWrapped.onServiceLost(serviceInfo)); } diff --git a/service-t/src/com/android/server/connectivity/mdns/MdnsDiscoveryManager.java b/service-t/src/com/android/server/connectivity/mdns/MdnsDiscoveryManager.java index 849eac13a9..6455044a13 100644 --- a/service-t/src/com/android/server/connectivity/mdns/MdnsDiscoveryManager.java +++ b/service-t/src/com/android/server/connectivity/mdns/MdnsDiscoveryManager.java @@ -135,18 +135,36 @@ public class MdnsDiscoveryManager implements MdnsSocketClientBase.Callback { } } // Request the network for discovery. - socketClient.notifyNetworkRequested(listener, searchOptions.getNetwork(), network -> { - synchronized (this) { - // All listeners of the same service types shares the same MdnsServiceTypeClient. - MdnsServiceTypeClient serviceTypeClient = - perNetworkServiceTypeClients.get(serviceType, network); - if (serviceTypeClient == null) { - serviceTypeClient = createServiceTypeClient(serviceType, network); - perNetworkServiceTypeClients.put(serviceType, network, serviceTypeClient); - } - serviceTypeClient.startSendAndReceive(listener, searchOptions); - } - }); + socketClient.notifyNetworkRequested(listener, searchOptions.getNetwork(), + new MdnsSocketClientBase.SocketCreationCallback() { + @Override + public void onSocketCreated(@Nullable Network network) { + synchronized (MdnsDiscoveryManager.this) { + // All listeners of the same service types shares the same + // MdnsServiceTypeClient. + MdnsServiceTypeClient serviceTypeClient = + perNetworkServiceTypeClients.get(serviceType, network); + if (serviceTypeClient == null) { + serviceTypeClient = createServiceTypeClient(serviceType, network); + perNetworkServiceTypeClients.put(serviceType, network, + serviceTypeClient); + } + serviceTypeClient.startSendAndReceive(listener, searchOptions); + } + } + + @Override + public void onSocketDestroyed(@Nullable Network network) { + synchronized (MdnsDiscoveryManager.this) { + final MdnsServiceTypeClient serviceTypeClient = + perNetworkServiceTypeClients.get(serviceType, network); + if (serviceTypeClient == null) return; + // Notify all listeners that all services are removed from this socket. + serviceTypeClient.notifyAllServicesRemoved(); + perNetworkServiceTypeClients.remove(serviceTypeClient); + } + } + }); } /** diff --git a/service-t/src/com/android/server/connectivity/mdns/MdnsMultinetworkSocketClient.java b/service-t/src/com/android/server/connectivity/mdns/MdnsMultinetworkSocketClient.java index 4504bb6a69..6414453cca 100644 --- a/service-t/src/com/android/server/connectivity/mdns/MdnsMultinetworkSocketClient.java +++ b/service-t/src/com/android/server/connectivity/mdns/MdnsMultinetworkSocketClient.java @@ -90,6 +90,7 @@ public class MdnsMultinetworkSocketClient implements MdnsSocketClientBase { @NonNull MdnsInterfaceSocket socket) { mSocketPacketHandlers.remove(socket); mActiveNetworkSockets.remove(socket); + mSocketCreationCallback.onSocketDestroyed(network); } } diff --git a/service-t/src/com/android/server/connectivity/mdns/MdnsServiceTypeClient.java b/service-t/src/com/android/server/connectivity/mdns/MdnsServiceTypeClient.java index 6585d4479e..35f9b0475d 100644 --- a/service-t/src/com/android/server/connectivity/mdns/MdnsServiceTypeClient.java +++ b/service-t/src/com/android/server/connectivity/mdns/MdnsServiceTypeClient.java @@ -263,6 +263,28 @@ public class MdnsServiceTypeClient { } } + /** Notify all services are removed because the socket is destroyed. */ + public void notifyAllServicesRemoved() { + synchronized (lock) { + for (MdnsResponse response : instanceNameToResponse.values()) { + final String name = response.getServiceInstanceName(); + if (name == null) continue; + for (int i = 0; i < listeners.size(); i++) { + if (!responseMatchesOptions(response, listeners.valueAt(i))) continue; + final MdnsServiceBrowserListener listener = listeners.keyAt(i); + final MdnsServiceInfo serviceInfo = + buildMdnsServiceInfoFromResponse(response, serviceTypeLabels); + if (response.isComplete()) { + sharedLog.log("Socket destroyed. onServiceRemoved: " + name); + listener.onServiceRemoved(serviceInfo); + } + sharedLog.log("Socket destroyed. onServiceNameRemoved: " + name); + listener.onServiceNameRemoved(serviceInfo); + } + } + } + } + private void onResponseModified(@NonNull MdnsResponse response) { final String serviceInstanceName = response.getServiceInstanceName(); final MdnsResponse currentResponse = diff --git a/service-t/src/com/android/server/connectivity/mdns/MdnsSocketClientBase.java b/service-t/src/com/android/server/connectivity/mdns/MdnsSocketClientBase.java index ebafc49c7c..6bcad01a82 100644 --- a/service-t/src/com/android/server/connectivity/mdns/MdnsSocketClientBase.java +++ b/service-t/src/com/android/server/connectivity/mdns/MdnsSocketClientBase.java @@ -86,5 +86,8 @@ public interface MdnsSocketClientBase { interface SocketCreationCallback { /*** Notify requested socket is created */ void onSocketCreated(@Nullable Network network); + + /*** Notify requested socket is destroyed */ + void onSocketDestroyed(@Nullable Network network); } } diff --git a/tests/unit/java/com/android/server/connectivity/mdns/MdnsDiscoveryManagerTests.java b/tests/unit/java/com/android/server/connectivity/mdns/MdnsDiscoveryManagerTests.java index 0a23ba5539..63357f1241 100644 --- a/tests/unit/java/com/android/server/connectivity/mdns/MdnsDiscoveryManagerTests.java +++ b/tests/unit/java/com/android/server/connectivity/mdns/MdnsDiscoveryManagerTests.java @@ -20,6 +20,7 @@ import static com.android.testutils.DevSdkIgnoreRuleKt.SC_V2; import static org.mockito.Mockito.eq; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -193,6 +194,60 @@ public class MdnsDiscoveryManagerTests { verify(mockServiceTypeClientTwo2).processResponse(responseForSubtype, ifIndex, NETWORK_2); } + @Test + public void testSocketCreatedAndDestroyed() throws IOException { + // Create a ServiceTypeClient for SERVICE_TYPE_1 and NETWORK_1 + final MdnsSearchOptions options1 = + MdnsSearchOptions.newBuilder().setNetwork(NETWORK_1).build(); + final SocketCreationCallback callback = expectSocketCreationCallback( + SERVICE_TYPE_1, mockListenerOne, options1); + callback.onSocketCreated(NETWORK_1); + verify(mockServiceTypeClientOne1).startSendAndReceive(mockListenerOne, options1); + + // Create a ServiceTypeClient for SERVICE_TYPE_2 and NETWORK_2 + final MdnsSearchOptions options2 = + MdnsSearchOptions.newBuilder().setNetwork(NETWORK_2).build(); + final SocketCreationCallback callback2 = expectSocketCreationCallback( + SERVICE_TYPE_2, mockListenerTwo, options2); + callback2.onSocketCreated(NETWORK_2); + verify(mockServiceTypeClientTwo2).startSendAndReceive(mockListenerTwo, options2); + + // Receive a response, it should be processed on both clients. + final MdnsPacket response = createMdnsPacket(SERVICE_TYPE_1); + final int ifIndex = 1; + discoveryManager.onResponseReceived(response, ifIndex, null /* network */); + verify(mockServiceTypeClientOne1).processResponse(response, ifIndex, null /* network */); + verify(mockServiceTypeClientTwo2).processResponse(response, ifIndex, null /* network */); + + // The client for NETWORK_1 receives the callback that the NETWORK_1 has been destroyed, + // mockServiceTypeClientOne1 should send service removed notifications and remove from the + // list of clients. + callback.onSocketDestroyed(NETWORK_1); + verify(mockServiceTypeClientOne1).notifyAllServicesRemoved(); + + // Receive a response again, it should be processed only on mockServiceTypeClientTwo2. + // Because the mockServiceTypeClientOne1 is removed from the list of clients, it is no + // longer able to process responses. + discoveryManager.onResponseReceived(response, ifIndex, null /* network */); + verify(mockServiceTypeClientOne1, times(1)) + .processResponse(response, ifIndex, null /* network */); + verify(mockServiceTypeClientTwo2, times(2)) + .processResponse(response, ifIndex, null /* network */); + + // The client for NETWORK_2 receives the callback that the NETWORK_1 has been destroyed, + // mockServiceTypeClientTwo2 shouldn't send any notifications. + callback2.onSocketDestroyed(NETWORK_1); + verify(mockServiceTypeClientTwo2, never()).notifyAllServicesRemoved(); + + // Receive a response again, mockServiceTypeClientTwo2 is still in the list of clients, it's + // still able to process responses. + discoveryManager.onResponseReceived(response, ifIndex, null /* network */); + verify(mockServiceTypeClientOne1, times(1)) + .processResponse(response, ifIndex, null /* network */); + verify(mockServiceTypeClientTwo2, times(3)) + .processResponse(response, ifIndex, null /* network */); + } + private MdnsPacket createMdnsPacket(String serviceType) { final String[] type = TextUtils.split(serviceType, "\\."); final ArrayList name = new ArrayList<>(type.length + 1); diff --git a/tests/unit/java/com/android/server/connectivity/mdns/MdnsServiceTypeClientTests.java b/tests/unit/java/com/android/server/connectivity/mdns/MdnsServiceTypeClientTests.java index 34b44fc1a9..2fcdff24d6 100644 --- a/tests/unit/java/com/android/server/connectivity/mdns/MdnsServiceTypeClientTests.java +++ b/tests/unit/java/com/android/server/connectivity/mdns/MdnsServiceTypeClientTests.java @@ -1056,6 +1056,61 @@ public class MdnsServiceTypeClientTests { inOrder.verify(mockListenerTwo).onServiceRemoved(matchServiceName(otherInstance)); } + @Test + public void testNotifyAllServicesRemoved() { + client = new MdnsServiceTypeClient( + SERVICE_TYPE, mockSocketClient, currentThreadExecutor, mockNetwork, mockSharedLog); + + final String requestedInstance = "instance1"; + final String otherInstance = "instance2"; + final String ipV4Address = "192.0.2.0"; + + final MdnsSearchOptions resolveOptions = MdnsSearchOptions.newBuilder() + // Use different case in the options + .setResolveInstanceName("Instance1").build(); + + client.startSendAndReceive(mockListenerOne, resolveOptions); + client.startSendAndReceive(mockListenerTwo, MdnsSearchOptions.getDefaultOptions()); + + // Complete response from instanceName + client.processResponse(createResponse( + requestedInstance, ipV4Address, 5353, SERVICE_TYPE_LABELS, + Collections.emptyMap() /* textAttributes */, TEST_TTL), + INTERFACE_INDEX, mockNetwork); + + // Complete response from otherInstanceName + client.processResponse(createResponse( + otherInstance, ipV4Address, 5353, SERVICE_TYPE_LABELS, + Collections.emptyMap() /* textAttributes */, TEST_TTL), + INTERFACE_INDEX, mockNetwork); + + client.notifyAllServicesRemoved(); + + // mockListenerOne gets notified for the requested instance + final InOrder inOrder1 = inOrder(mockListenerOne); + inOrder1.verify(mockListenerOne).onServiceNameDiscovered( + matchServiceName(requestedInstance)); + inOrder1.verify(mockListenerOne).onServiceFound(matchServiceName(requestedInstance)); + inOrder1.verify(mockListenerOne).onServiceRemoved(matchServiceName(requestedInstance)); + inOrder1.verify(mockListenerOne).onServiceNameRemoved(matchServiceName(requestedInstance)); + verify(mockListenerOne, never()).onServiceFound(matchServiceName(otherInstance)); + verify(mockListenerOne, never()).onServiceNameDiscovered(matchServiceName(otherInstance)); + verify(mockListenerOne, never()).onServiceRemoved(matchServiceName(otherInstance)); + verify(mockListenerOne, never()).onServiceNameRemoved(matchServiceName(otherInstance)); + + // mockListenerTwo gets notified for both though + final InOrder inOrder2 = inOrder(mockListenerTwo); + inOrder2.verify(mockListenerTwo).onServiceNameDiscovered( + matchServiceName(requestedInstance)); + inOrder2.verify(mockListenerTwo).onServiceFound(matchServiceName(requestedInstance)); + inOrder2.verify(mockListenerTwo).onServiceNameDiscovered(matchServiceName(otherInstance)); + inOrder2.verify(mockListenerTwo).onServiceFound(matchServiceName(otherInstance)); + inOrder2.verify(mockListenerTwo).onServiceRemoved(matchServiceName(otherInstance)); + inOrder2.verify(mockListenerTwo).onServiceNameRemoved(matchServiceName(otherInstance)); + inOrder2.verify(mockListenerTwo).onServiceRemoved(matchServiceName(requestedInstance)); + inOrder2.verify(mockListenerTwo).onServiceNameRemoved(matchServiceName(requestedInstance)); + } + private static MdnsServiceInfo matchServiceName(String name) { return argThat(info -> info.getServiceInstanceName().equals(name)); }