diff --git a/src/lib/mdns/Resolver_ImplMinimalMdns.cpp b/src/lib/mdns/Resolver_ImplMinimalMdns.cpp index bdb23dcf19b7d9..9f4fa1632e4c9f 100644 --- 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 index 666c579914190c..1cec24734f6fb5 100644 --- 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 index aa5a60a8f46ef9..9f05c7b9dc8148 100644 --- 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 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 index e320fa210c3c87..65f04e07cd0f7f 100644 --- 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::Value(MakePeerId(1))); + NL_TEST_ASSERT(inSuite, !attempts.NextScheduledPeer().HasValue()); + NL_TEST_ASSERT(inSuite, attempts.GetMsUntilNextExpectedResponse() == Optional::Value(1000u)); + mockClock.AdvanceMs(20); + NL_TEST_ASSERT(inSuite, attempts.GetMsUntilNextExpectedResponse() == Optional::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::Value(MakePeerId(2))); + NL_TEST_ASSERT(inSuite, !attempts.NextScheduledPeer().HasValue()); + mockClock.AdvanceMs(80); + NL_TEST_ASSERT(inSuite, attempts.GetMsUntilNextExpectedResponse() == Optional::Value(900u)); + + // Peer 1 is done, now peer2 should be pending (in 980ms) + attempts.Complete(MakePeerId(1)); + NL_TEST_ASSERT(inSuite, attempts.GetMsUntilNextExpectedResponse() == Optional::Value(920u)); + mockClock.AdvanceMs(20); + NL_TEST_ASSERT(inSuite, attempts.GetMsUntilNextExpectedResponse() == Optional::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::Value(MakePeerId(3))); + NL_TEST_ASSERT(inSuite, !attempts.NextScheduledPeer().HasValue()); + NL_TEST_ASSERT(inSuite, attempts.GetMsUntilNextExpectedResponse() == Optional::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::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::Value(MakePeerId(3))); + NL_TEST_ASSERT(inSuite, attempts.NextScheduledPeer() == Optional::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() // };