Code review updates, add a more complex test for scheduling check

project-chip · Sep 27, 2021 · 8bf12cc · 8bf12cc
1 parent b2714c7
commit 8bf12cc
Show file tree

Hide file tree

Showing 4 changed files with 76 additions and 6 deletions.
diff --git a/src/lib/mdns/Resolver_ImplMinimalMdns.cpp b/src/lib/mdns/Resolver_ImplMinimalMdns.cpp
@@ -86,7 +86,7 @@ class PacketDataReporter : public ParserDelegate
 
     // Called after ParsePacket is complete to send final notifications to the delegate.
     // Used to ensure all the available IP addresses are attached before completion.
-    void OnComplete(ActiveResolveAttempts & ActiveResolveAttempts);
+    void OnComplete(ActiveResolveAttempts & activeAttempts);
 
 private:
     ResolverDelegate * mDelegate = nullptr;
@@ -552,7 +552,7 @@ CHIP_ERROR MinMdnsResolver::SendPendingResolveQueries()
         builder.Header().SetMessageId(0);
 
         {
-            char nameBuffer[64] = "";
+            char nameBuffer[kMaxOperationalServiceNameSize] = "";
 
             // Node and fabricid are encoded in server names.
             ReturnErrorOnFailure(MakeInstanceName(nameBuffer, sizeof(nameBuffer), peerId.Value()));

diff --git a/src/lib/mdns/minimal/ActiveResolveAttempts.cpp b/src/lib/mdns/minimal/ActiveResolveAttempts.cpp
@@ -89,8 +89,11 @@ void ActiveResolveAttempts::MarkPending(const PeerId & peerId)
             continue;
         }
 
-        // Rule 2: both choices are used (have a defined node id),
-        // pick the one with most queryDueTimeMs
+        // Rule 3: both choices are used (have a defined node id):
+        //    - try to find the one with the largest next delay (oldest request)
+        //    - on same delay, use queryDueTime to determine the oldest request
+        //      (the one with the smallest  due time was issued the longest time
+        //       ago)
         if (entry->nextRetryDelaySec > entryToUse->nextRetryDelaySec)
         {
             entryToUse = entry;
@@ -104,6 +107,13 @@ void ActiveResolveAttempts::MarkPending(const PeerId & peerId)
 
     if ((entryToUse->peerId.GetNodeId() != kUndefinedNodeId) && (entryToUse->peerId != peerId))
     {
+        // TODO: node was evicted here, if/when resolution failures are
+        // supported this could be a place for error callbacks
+        //
+        // Note however that this is NOT an actual 'timeout' it is showing
+        // a burst of lookups for which we cannot maintain state. A reply may
+        // still be received for this peer id (query was already sent on the
+        // network)
         ChipLogError(Discovery, "Re-using pending resolve entry before reply was received.");
     }
 

diff --git a/src/lib/mdns/minimal/ActiveResolveAttempts.h b/src/lib/mdns/minimal/ActiveResolveAttempts.h
@@ -63,13 +63,17 @@ class ActiveResolveAttempts
     // Get the peer Id that needs scheduling for a query
     //
     // Assumes that the resolution is being sent and will apply internal
-    // query logic.
+    // query logic. This means:
+    //  - internal tracking of 'next due time' will updated as 'request sent
+    //    now'
+    //  - there is NO sorting implied by this call. Returned value will be
+    //    any peer that needs a new request sent
     chip::Optional<chip::PeerId> NextScheduledPeer();
 
 private:
     struct RetryEntry
     {
-        // What peer id is pending discovery.
+        // What peer id is pending resolution.
         //
         // Inactive entries are marked by having NodeId == kUndefinedNodeId
         chip::PeerId peerId;

diff --git a/src/lib/mdns/minimal/tests/TestActiveResolveAttempts.cpp b/src/lib/mdns/minimal/tests/TestActiveResolveAttempts.cpp
@@ -193,10 +193,66 @@ void TestLRU(nlTestSuite * inSuite, void * inContext)
     NL_TEST_ASSERT(inSuite, i < kMaxIterations);
 }
 
+void TestNextPeerOrdering(nlTestSuite * inSuite, void * inContext)
+{
+    MockClock mockClock;
+    mdns::Minimal::ActiveResolveAttempts attempts(&mockClock);
+
+    mockClock.AdvanceMs(123321);
+
+    // add a single peer that will be resolved quickly
+    attempts.MarkPending(MakePeerId(1));
+
+    NL_TEST_ASSERT(inSuite, attempts.NextScheduledPeer() == Optional<PeerId>::Value(MakePeerId(1)));
+    NL_TEST_ASSERT(inSuite, !attempts.NextScheduledPeer().HasValue());
+    NL_TEST_ASSERT(inSuite, attempts.GetMsUntilNextExpectedResponse() == Optional<uint32_t>::Value(1000u));
+    mockClock.AdvanceMs(20);
+    NL_TEST_ASSERT(inSuite, attempts.GetMsUntilNextExpectedResponse() == Optional<uint32_t>::Value(980u));
+
+    // expect peerid to be resolve within 1 second from now
+    attempts.MarkPending(MakePeerId(2));
+
+    // mock that we are querying 2 as well
+    NL_TEST_ASSERT(inSuite, attempts.NextScheduledPeer() == Optional<PeerId>::Value(MakePeerId(2)));
+    NL_TEST_ASSERT(inSuite, !attempts.NextScheduledPeer().HasValue());
+    mockClock.AdvanceMs(80);
+    NL_TEST_ASSERT(inSuite, attempts.GetMsUntilNextExpectedResponse() == Optional<uint32_t>::Value(900u));
+
+    // Peer 1 is done, now peer2 should be pending (in 980ms)
+    attempts.Complete(MakePeerId(1));
+    NL_TEST_ASSERT(inSuite, attempts.GetMsUntilNextExpectedResponse() == Optional<uint32_t>::Value(920u));
+    mockClock.AdvanceMs(20);
+    NL_TEST_ASSERT(inSuite, attempts.GetMsUntilNextExpectedResponse() == Optional<uint32_t>::Value(900u));
+
+    // Once peer 3 is added, queue should be
+    //  - 900 ms until peer id 2 is pending
+    //  - 1000 ms until peer id 3 is pending
+    attempts.MarkPending(MakePeerId(3));
+    NL_TEST_ASSERT(inSuite, attempts.NextScheduledPeer() == Optional<PeerId>::Value(MakePeerId(3)));
+    NL_TEST_ASSERT(inSuite, !attempts.NextScheduledPeer().HasValue());
+    NL_TEST_ASSERT(inSuite, attempts.GetMsUntilNextExpectedResponse() == Optional<uint32_t>::Value(900u));
+
+    // After the clock advance
+    //  - 400 ms until peer id 2 is pending
+    //  - 500 ms until peer id 3 is pending
+    mockClock.AdvanceMs(500);
+
+    NL_TEST_ASSERT(inSuite, attempts.GetMsUntilNextExpectedResponse() == Optional<uint32_t>::Value(400u));
+    NL_TEST_ASSERT(inSuite, !attempts.NextScheduledPeer().HasValue());
+
+    // advancing the clock 'too long' will return both other entries, in  reverse order due to how
+    // the internal cache is built
+    mockClock.AdvanceMs(500);
+    NL_TEST_ASSERT(inSuite, attempts.NextScheduledPeer() == Optional<PeerId>::Value(MakePeerId(3)));
+    NL_TEST_ASSERT(inSuite, attempts.NextScheduledPeer() == Optional<PeerId>::Value(MakePeerId(2)));
+    NL_TEST_ASSERT(inSuite, !attempts.NextScheduledPeer().HasValue());
+}
+
 const nlTest sTests[] = {
     NL_TEST_DEF("TestSinglePeerAddRemove", TestSinglePeerAddRemove),   //
     NL_TEST_DEF("TestRescheduleSamePeerId", TestRescheduleSamePeerId), //
     NL_TEST_DEF("TestLRU", TestLRU),                                   //
+    NL_TEST_DEF("TestNextPeerOrdering", TestNextPeerOrdering),         //
     NL_TEST_SENTINEL()                                                 //
 };