From 86f674187ff176941341dc2dc4c450f1bd2979e4 Mon Sep 17 00:00:00 2001 From: Yuyang Huang Date: Mon, 5 Jun 2023 16:24:35 +0900 Subject: [PATCH] Update record receipt time on records updated The receipt time in the MdnsResponse need to be updated every time new packet is received. And then the record refreshing logic could calculate remaining TTL correctly. Bug: 285260665 Test: atest CtsNetTest FrameworksNetTests Change-Id: Ib7a290ea0ea8e552c71c657696397e8114fcee52 --- .../connectivity/mdns/MdnsResponse.java | 19 +++--- .../mdns/MdnsResponseDecoder.java | 13 ++-- .../mdns/MdnsServiceTypeClient.java | 38 ++++++++---- .../mdns/MdnsResponseDecoderTests.java | 4 +- .../connectivity/mdns/MdnsResponseTests.java | 60 +++++++++++++++---- .../mdns/MdnsServiceTypeClientTests.java | 34 +++++++++++ 6 files changed, 130 insertions(+), 38 deletions(-) diff --git a/service-t/src/com/android/server/connectivity/mdns/MdnsResponse.java b/service-t/src/com/android/server/connectivity/mdns/MdnsResponse.java index 28aa640d96..ec1e4627fe 100644 --- a/service-t/src/com/android/server/connectivity/mdns/MdnsResponse.java +++ b/service-t/src/com/android/server/connectivity/mdns/MdnsResponse.java @@ -84,16 +84,15 @@ public class MdnsResponse { private boolean addOrReplaceRecord(@NonNull T record, @NonNull List recordsList) { final int existing = recordsList.indexOf(record); + boolean isSame = false; if (existing >= 0) { - if (recordsAreSame(record, recordsList.get(existing))) { - return false; - } + isSame = recordsAreSame(record, recordsList.get(existing)); final MdnsRecord existedRecord = recordsList.remove(existing); records.remove(existedRecord); } recordsList.add(record); records.add(record); - return true; + return !isSame; } /** @@ -163,9 +162,7 @@ public class MdnsResponse { /** Sets the service record. */ public synchronized boolean setServiceRecord(MdnsServiceRecord serviceRecord) { - if (recordsAreSame(this.serviceRecord, serviceRecord)) { - return false; - } + boolean isSame = recordsAreSame(this.serviceRecord, serviceRecord); if (this.serviceRecord != null) { records.remove(this.serviceRecord); } @@ -173,7 +170,7 @@ public class MdnsResponse { if (this.serviceRecord != null) { records.add(this.serviceRecord); } - return true; + return !isSame; } /** Gets the service record. */ @@ -187,9 +184,7 @@ public class MdnsResponse { /** Sets the text record. */ public synchronized boolean setTextRecord(MdnsTextRecord textRecord) { - if (recordsAreSame(this.textRecord, textRecord)) { - return false; - } + boolean isSame = recordsAreSame(this.textRecord, textRecord); if (this.textRecord != null) { records.remove(this.textRecord); } @@ -197,7 +192,7 @@ public class MdnsResponse { if (this.textRecord != null) { records.add(this.textRecord); } - return true; + return !isSame; } /** Gets the text record. */ diff --git a/service-t/src/com/android/server/connectivity/mdns/MdnsResponseDecoder.java b/service-t/src/com/android/server/connectivity/mdns/MdnsResponseDecoder.java index 77b5c58d44..42f610771a 100644 --- a/service-t/src/com/android/server/connectivity/mdns/MdnsResponseDecoder.java +++ b/service-t/src/com/android/server/connectivity/mdns/MdnsResponseDecoder.java @@ -21,6 +21,7 @@ import android.annotation.Nullable; import android.net.Network; import android.os.SystemClock; import android.util.ArraySet; +import android.util.Pair; import com.android.server.connectivity.mdns.util.MdnsLogger; import com.android.server.connectivity.mdns.util.MdnsUtils; @@ -120,9 +121,14 @@ public class MdnsResponseDecoder { * @param interfaceIndex the network interface index (or * {@link MdnsSocket#INTERFACE_INDEX_UNSPECIFIED} if not known) at which the packet was received * @param network the network at which the packet was received, or null if it is unknown. - * @return The set of response instances that were modified or newly added. + * @return The pair of 1) set of response instances that were modified or newly added. *not* + * including those which records were only updated with newer receive + * timestamps. + * 2) A copy of the original responses with some of them have records + * update or only contains receive time updated. */ - public ArraySet augmentResponses(@NonNull MdnsPacket mdnsPacket, + public Pair, ArrayList> augmentResponses( + @NonNull MdnsPacket mdnsPacket, @NonNull Collection existingResponses, int interfaceIndex, @Nullable Network network) { final ArrayList records = new ArrayList<>( @@ -177,7 +183,6 @@ public class MdnsResponseDecoder { network); responses.add(response); } - if (response.addPointerRecord((MdnsPointerRecord) record)) { modified.add(response); } @@ -269,7 +274,7 @@ public class MdnsResponseDecoder { } } - return modified; + return Pair.create(modified, responses); } private static boolean assignInetRecord( 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 809750da63..14302c2d8d 100644 --- a/service-t/src/com/android/server/connectivity/mdns/MdnsServiceTypeClient.java +++ b/service-t/src/com/android/server/connectivity/mdns/MdnsServiceTypeClient.java @@ -63,6 +63,7 @@ public class MdnsServiceTypeClient { private final Object lock = new Object(); private final ArrayMap listeners = new ArrayMap<>(); + // TODO: change instanceNameToResponse to TreeMap with case insensitive comparator. private final Map instanceNameToResponse = new HashMap<>(); private final boolean removeServiceAfterTtlExpires = MdnsConfigs.removeServiceAfterTtlExpires(); @@ -260,15 +261,32 @@ public class MdnsServiceTypeClient { // Augment the list of current known responses, and generated responses for resolve // requests if there is no known response final List currentList = new ArrayList<>(instanceNameToResponse.values()); - currentList.addAll(makeResponsesForResolveIfUnknown(interfaceIndex, network)); - final ArraySet modifiedResponses = responseDecoder.augmentResponses( - packet, currentList, interfaceIndex, network); - for (MdnsResponse modified : modifiedResponses) { - if (modified.isGoodbye()) { - onGoodbyeReceived(modified.getServiceInstanceName()); - } else { - onResponseModified(modified); + List additionalResponses = makeResponsesForResolve(interfaceIndex, + network); + for (MdnsResponse additionalResponse : additionalResponses) { + if (!instanceNameToResponse.containsKey( + additionalResponse.getServiceInstanceName())) { + currentList.add(additionalResponse); + } + } + final Pair, ArrayList> augmentedResult = + responseDecoder.augmentResponses(packet, currentList, interfaceIndex, network); + + final ArraySet modifiedResponse = augmentedResult.first; + final ArrayList allResponses = augmentedResult.second; + + for (MdnsResponse response : allResponses) { + if (modifiedResponse.contains(response)) { + if (response.isGoodbye()) { + onGoodbyeReceived(response.getServiceInstanceName()); + } else { + onResponseModified(response); + } + } else if (instanceNameToResponse.containsKey(response.getServiceInstanceName())) { + // If the response is not modified and already in the cache. The cache will + // need to be updated to refresh the last receipt time. + instanceNameToResponse.put(response.getServiceInstanceName(), response); } } } @@ -474,7 +492,7 @@ public class MdnsServiceTypeClient { } } - private List makeResponsesForResolveIfUnknown(int interfaceIndex, + private List makeResponsesForResolve(int interfaceIndex, @NonNull Network network) { final List resolveResponses = new ArrayList<>(); for (int i = 0; i < listeners.size(); i++) { @@ -516,7 +534,7 @@ public class MdnsServiceTypeClient { // queried to complete it. // Only the names are used to know which queries to send, other parameters like // interfaceIndex do not matter. - servicesToResolve = makeResponsesForResolveIfUnknown( + servicesToResolve = makeResponsesForResolve( 0 /* interfaceIndex */, config.network); sendDiscoveryQueries = servicesToResolve.size() < listeners.size(); } diff --git a/tests/unit/java/com/android/server/connectivity/mdns/MdnsResponseDecoderTests.java b/tests/unit/java/com/android/server/connectivity/mdns/MdnsResponseDecoderTests.java index e16c448de6..6eb83da639 100644 --- a/tests/unit/java/com/android/server/connectivity/mdns/MdnsResponseDecoderTests.java +++ b/tests/unit/java/com/android/server/connectivity/mdns/MdnsResponseDecoderTests.java @@ -344,7 +344,7 @@ public class MdnsResponseDecoderTests { final Network network = mock(Network.class); responses = decoder.augmentResponses(parsedPacket, /* existingResponses= */ Collections.emptyList(), - /* interfaceIndex= */ 10, network /* expireOnExit= */); + /* interfaceIndex= */ 10, network /* expireOnExit= */).first; assertEquals(responses.size(), 1); assertEquals(responses.valueAt(0).getInterfaceIndex(), 10); @@ -593,6 +593,6 @@ public class MdnsResponseDecoderTests { return decoder.augmentResponses(parsedPacket, existingResponses, - MdnsSocket.INTERFACE_INDEX_UNSPECIFIED, mock(Network.class)); + MdnsSocket.INTERFACE_INDEX_UNSPECIFIED, mock(Network.class)).first; } } \ No newline at end of file diff --git a/tests/unit/java/com/android/server/connectivity/mdns/MdnsResponseTests.java b/tests/unit/java/com/android/server/connectivity/mdns/MdnsResponseTests.java index dc0e64650f..3e189f12fc 100644 --- a/tests/unit/java/com/android/server/connectivity/mdns/MdnsResponseTests.java +++ b/tests/unit/java/com/android/server/connectivity/mdns/MdnsResponseTests.java @@ -95,24 +95,24 @@ public class MdnsResponseTests { } } - private MdnsResponse makeCompleteResponse(int recordsTtlMillis) { + private MdnsResponse makeCompleteResponse(int recordsTtlMillis, int receiptTimeMillis) { final String[] hostname = new String[] { "MyHostname" }; final String[] serviceName = new String[] { "MyService", "_type", "_tcp", "local" }; final String[] serviceType = new String[] { "_type", "_tcp", "local" }; final MdnsResponse response = new MdnsResponse(/* now= */ 0, serviceName, INTERFACE_INDEX, mNetwork); - response.addPointerRecord(new MdnsPointerRecord(serviceType, 0L /* receiptTimeMillis */, + response.addPointerRecord(new MdnsPointerRecord(serviceType, receiptTimeMillis, false /* cacheFlush */, recordsTtlMillis, serviceName)); - response.setServiceRecord(new MdnsServiceRecord(serviceName, 0L /* receiptTimeMillis */, + response.setServiceRecord(new MdnsServiceRecord(serviceName, receiptTimeMillis, true /* cacheFlush */, recordsTtlMillis, 0 /* servicePriority */, 0 /* serviceWeight */, 0 /* servicePort */, hostname)); - response.setTextRecord(new MdnsTextRecord(serviceName, 0L /* receiptTimeMillis */, + response.setTextRecord(new MdnsTextRecord(serviceName, receiptTimeMillis, true /* cacheFlush */, recordsTtlMillis, emptyList() /* entries */)); response.addInet4AddressRecord(new MdnsInetAddressRecord( - hostname, 0L /* receiptTimeMillis */, true /* cacheFlush */, + hostname, receiptTimeMillis, true /* cacheFlush */, recordsTtlMillis, parseNumericAddress("192.0.2.123"))); response.addInet6AddressRecord(new MdnsInetAddressRecord( - hostname, 0L /* receiptTimeMillis */, true /* cacheFlush */, + hostname, receiptTimeMillis, true /* cacheFlush */, recordsTtlMillis, parseNumericAddress("2001:db8::123"))); return response; } @@ -210,7 +210,7 @@ public class MdnsResponseTests { @Test public void copyConstructor() { - final MdnsResponse response = makeCompleteResponse(TEST_TTL_MS); + final MdnsResponse response = makeCompleteResponse(TEST_TTL_MS, 0 /* receiptTimeMillis */); final MdnsResponse copy = new MdnsResponse(response); assertEquals(response.getInet6AddressRecord(), copy.getInet6AddressRecord()); @@ -225,7 +225,7 @@ public class MdnsResponseTests { @Test public void addRecords_noChange() { - final MdnsResponse response = makeCompleteResponse(TEST_TTL_MS); + final MdnsResponse response = makeCompleteResponse(TEST_TTL_MS, 0 /* receiptTimeMillis */); assertFalse(response.addPointerRecord(response.getPointerRecords().get(0))); final String[] serviceName = new String[] { "MYSERVICE", "_TYPE", "_tcp", "local" }; @@ -242,8 +242,8 @@ public class MdnsResponseTests { @Test public void addRecords_ttlChange() { - final MdnsResponse response = makeCompleteResponse(TEST_TTL_MS); - final MdnsResponse ttlZeroResponse = makeCompleteResponse(0); + final MdnsResponse response = makeCompleteResponse(TEST_TTL_MS, 0 /* receiptTimeMillis */); + final MdnsResponse ttlZeroResponse = makeCompleteResponse(0, 0 /* receiptTimeMillis */); assertTrue(response.addPointerRecord(ttlZeroResponse.getPointerRecords().get(0))); assertEquals(1, response.getPointerRecords().size()); @@ -277,6 +277,46 @@ public class MdnsResponseTests { assertEquals(ttlZeroResponse.getRecords().size(), response.getRecords().size()); } + @Test + public void addRecords_receiptTimeChange() { + final MdnsResponse response = makeCompleteResponse(TEST_TTL_MS, 0 /* receiptTimeMillis */); + final MdnsResponse receiptTimeChangedResponse = makeCompleteResponse(TEST_TTL_MS, + 1 /* receiptTimeMillis */); + + assertFalse( + response.addPointerRecord(receiptTimeChangedResponse.getPointerRecords().get(0))); + assertEquals(1, response.getPointerRecords().get(0).getReceiptTime()); + assertTrue(response.getRecords().stream().anyMatch(r -> + r == response.getPointerRecords().get(0))); + + assertFalse( + response.addInet6AddressRecord(receiptTimeChangedResponse.getInet6AddressRecord())); + assertEquals(1, response.getInet6AddressRecords().size()); + assertEquals(1, response.getInet6AddressRecord().getReceiptTime()); + assertTrue(response.getRecords().stream().anyMatch(r -> + r == response.getInet6AddressRecord())); + + assertFalse( + response.addInet4AddressRecord(receiptTimeChangedResponse.getInet4AddressRecord())); + assertEquals(1, response.getInet4AddressRecords().size()); + assertEquals(1, response.getInet4AddressRecord().getReceiptTime()); + assertTrue(response.getRecords().stream().anyMatch(r -> + r == response.getInet4AddressRecord())); + + assertFalse(response.setServiceRecord(receiptTimeChangedResponse.getServiceRecord())); + assertEquals(1, response.getServiceRecord().getReceiptTime()); + assertTrue(response.getRecords().stream().anyMatch(r -> + r == response.getServiceRecord())); + + assertFalse(response.setTextRecord(receiptTimeChangedResponse.getTextRecord())); + assertEquals(1, response.getTextRecord().getReceiptTime()); + assertTrue(response.getRecords().stream().anyMatch(r -> + r == response.getTextRecord())); + + // All records were replaced, not added + assertEquals(receiptTimeChangedResponse.getRecords().size(), response.getRecords().size()); + } + @Test public void dropUnmatchedAddressRecords_caseInsensitive() { 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 da51240c9f..a696150cff 100644 --- a/tests/unit/java/com/android/server/connectivity/mdns/MdnsServiceTypeClientTests.java +++ b/tests/unit/java/com/android/server/connectivity/mdns/MdnsServiceTypeClientTests.java @@ -25,6 +25,7 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doReturn; @@ -1065,10 +1066,43 @@ public class MdnsServiceTypeClientTests { // Second and later sends are sent as "expect multicast response" queries inOrder.verify(mockSocketClient, times(2)).sendMulticastPacket(renewalQueryCaptor.capture(), eq(mockNetwork)); + inOrder.verify(mockListenerOne).onDiscoveryQuerySent(any(), anyInt()); final MdnsPacket renewalPacket = MdnsPacket.parse( new MdnsPacketReader(renewalQueryCaptor.getValue())); assertTrue(hasQuestion(renewalPacket, MdnsRecord.TYPE_SRV, serviceName)); assertTrue(hasQuestion(renewalPacket, MdnsRecord.TYPE_TXT, serviceName)); + inOrder.verifyNoMoreInteractions(); + + long updatedReceiptTime = TEST_ELAPSED_REALTIME + TEST_TTL; + final MdnsPacket refreshedSrvTxtResponse = new MdnsPacket( + 0 /* flags */, + Collections.emptyList() /* questions */, + List.of( + // TODO: cacheFlush will cause addresses to be cleared and re-added every + // time, which is considered a change and triggers extra + // onServiceChanged callbacks. Sets cacheFlush bit to false until the + // issue is fixed. + new MdnsServiceRecord(serviceName, updatedReceiptTime, + false /* cacheFlush */, TEST_TTL, 0 /* servicePriority */, + 0 /* serviceWeight */, 1234 /* servicePort */, hostname), + new MdnsTextRecord(serviceName, updatedReceiptTime, + false /* cacheFlush */, TEST_TTL, + Collections.emptyList() /* entries */), + new MdnsInetAddressRecord(hostname, updatedReceiptTime, + false /* cacheFlush */, TEST_TTL, + InetAddresses.parseNumericAddress(ipV4Address)), + new MdnsInetAddressRecord(hostname, updatedReceiptTime, + false /* cacheFlush */, TEST_TTL, + InetAddresses.parseNumericAddress(ipV6Address))), + Collections.emptyList() /* authorityRecords */, + Collections.emptyList() /* additionalRecords */); + client.processResponse(refreshedSrvTxtResponse, INTERFACE_INDEX, mockNetwork); + + // Advance time to updatedReceiptTime + 1, expected no refresh query because the cache + // should contain the record that have update last receipt time. + doReturn(updatedReceiptTime + 1).when(mockDecoderClock).elapsedRealtime(); + currentThreadExecutor.getAndClearLastScheduledRunnable().run(); + inOrder.verifyNoMoreInteractions(); } @Test