diff --git a/daemon/fw/multicast-strategy.cpp b/daemon/fw/multicast-strategy.cpp index 244ccfcd..ee921c37 100644 --- a/daemon/fw/multicast-strategy.cpp +++ b/daemon/fw/multicast-strategy.cpp @@ -69,10 +69,6 @@ MulticastStrategy::afterReceiveInterest(const FaceEndpoint& ingress, const Inter const fib::Entry& fibEntry = this->lookupFib(*pitEntry); const fib::NextHopList& nexthops = fibEntry.getNextHops(); - int nEligibleNextHops = 0; - - bool isSuppressed = false; - for (const auto& nexthop : nexthops) { Face& outFace = nexthop.getFace(); @@ -80,12 +76,10 @@ MulticastStrategy::afterReceiveInterest(const FaceEndpoint& ingress, const Inter if (suppressResult == RetxSuppressionResult::SUPPRESS) { NFD_LOG_DEBUG(interest << " from=" << ingress << " to=" << outFace.getId() << " suppressed"); - isSuppressed = true; continue; } - if ((outFace.getId() == ingress.face.getId() && outFace.getLinkType() != ndn::nfd::LINK_TYPE_AD_HOC) || - wouldViolateScope(ingress.face, interest, outFace)) { + if (!isNextHopEligible(ingress.face, interest, nexthop, pitEntry)) { continue; } @@ -95,11 +89,10 @@ MulticastStrategy::afterReceiveInterest(const FaceEndpoint& ingress, const Inter if (suppressResult == RetxSuppressionResult::FORWARD) { m_retxSuppression.incrementIntervalForOutRecord(*pitEntry->getOutRecord(outFace)); } - ++nEligibleNextHops; } - if (nEligibleNextHops == 0 && !isSuppressed) { - NFD_LOG_DEBUG(interest << " from=" << ingress << " noNextHop"); + if (!hasPendingOutRecords(*pitEntry)) { + NFD_LOG_DEBUG(interest << " from=" << ingress << " noNextHop (removing pitEntry)"); lp::NackHeader nackHeader; nackHeader.setReason(lp::NackReason::NO_ROUTE); diff --git a/tests/daemon/fw/multicast-strategy.t.cpp b/tests/daemon/fw/multicast-strategy.t.cpp index 526d7b0f..6871821f 100644 --- a/tests/daemon/fw/multicast-strategy.t.cpp +++ b/tests/daemon/fw/multicast-strategy.t.cpp @@ -1,6 +1,6 @@ /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */ /* - * Copyright (c) 2014-2019, Regents of the University of California, + * Copyright (c) 2014-2020, Regents of the University of California, * Arizona Board of Regents, * Colorado State University, * University Pierre & Marie Curie, Sorbonne University, @@ -29,6 +29,7 @@ #include "tests/test-common.hpp" #include "tests/daemon/face/dummy-face.hpp" #include "strategy-tester.hpp" +#include "topology-tester.hpp" namespace nfd { namespace fw { @@ -65,6 +66,81 @@ protected: BOOST_AUTO_TEST_SUITE(Fw) BOOST_FIXTURE_TEST_SUITE(TestMulticastStrategy, MulticastStrategyFixture) +BOOST_AUTO_TEST_CASE(Bug5123) +{ + fib::Entry& fibEntry = *fib.insert(Name()).first; + fib.addOrUpdateNextHop(fibEntry, *face2, 0); + + // Send an Interest from face 1 to face 2 + shared_ptr interest = makeInterest("ndn:/H0D6i5fc"); + shared_ptr pitEntry = pit.insert(*interest).first; + pitEntry->insertOrUpdateInRecord(*face1, *interest); + + strategy.afterReceiveInterest(FaceEndpoint(*face1, 0), *interest, pitEntry); + BOOST_CHECK_EQUAL(strategy.rejectPendingInterestHistory.size(), 0); + + // Advance more than default suppression + this->advanceClocks(15_ms); + + // Get same interest from face 2 which does not have anywhere to go + pitEntry = pit.insert(*interest).first; + pitEntry->insertOrUpdateInRecord(*face2, *interest); + + strategy.afterReceiveInterest(FaceEndpoint(*face2, 0), *interest, pitEntry); + // Since the interest is same as the one sent out by face 1 pit should not be rejected + // as any data coming back should be able to satisfy original interest from face 1 + BOOST_CHECK_EQUAL(strategy.rejectPendingInterestHistory.size(), 0); + + /* + * +---------+ +---------+ +---------+ + * | nodeA |------------| nodeB |----------| nodeC | + * +---------+ 10ms +---------+ 100ms +---------+ + */ + + const Name PRODUCER_PREFIX = "/ndn/edu/nodeC/ping"; + + TopologyTester topo; + TopologyNode nodeA = topo.addForwarder("A"), + nodeB = topo.addForwarder("B"), + nodeC = topo.addForwarder("C"); + + for (TopologyNode node : {nodeA, nodeB, nodeC}) { + topo.setStrategy(node); + } + + shared_ptr linkAB = topo.addLink("AB", 10_ms, {nodeA, nodeB}), + linkBC = topo.addLink("BC", 100_ms, {nodeB, nodeC}); + + shared_ptr appA = topo.addAppFace("cA", nodeA), + appB = topo.addAppFace("cB", nodeB), + pingServer = topo.addAppFace("p", nodeC, PRODUCER_PREFIX); + topo.addEchoProducer(pingServer->getClientFace()); + topo.registerPrefix(nodeA, linkAB->getFace(nodeA), PRODUCER_PREFIX, 10); + topo.registerPrefix(nodeB, linkAB->getFace(nodeB), PRODUCER_PREFIX, 10); + topo.registerPrefix(nodeB, linkBC->getFace(nodeB), PRODUCER_PREFIX, 100); + + Name name(PRODUCER_PREFIX); + name.appendTimestamp(); + interest = makeInterest(name); + appA->getClientFace().expressInterest(*interest, nullptr, nullptr, nullptr); + + this->advanceClocks(10_ms, 20_ms); + + // AppB expresses the same interest + interest->refreshNonce(); + appB->getClientFace().expressInterest(*interest, nullptr, nullptr, nullptr); + this->advanceClocks(10_ms, 200_ms); + + // Data should have made to appB + BOOST_CHECK_EQUAL(linkBC->getFace(nodeB).getCounters().nInData, 1); + BOOST_CHECK_EQUAL(linkAB->getFace(nodeA).getCounters().nInData, 0); + + this->advanceClocks(10_ms, 10_ms); + // nodeA should have gotten the data successfully + BOOST_CHECK_EQUAL(linkAB->getFace(nodeA).getCounters().nInData, 1); + BOOST_CHECK_EQUAL(topo.getForwarder(nodeA).getCounters().nUnsolicitedData, 0); +} + BOOST_AUTO_TEST_CASE(Forward2) { fib::Entry& fibEntry = *fib.insert(Name()).first;