fw: fix PIT entry rejection in MulticastStrategy on receiving same Interest
refs: #5123
Change-Id: Ie0e1ff798313b190618a42b2a78f8b0cc4184097
diff --git a/daemon/fw/multicast-strategy.cpp b/daemon/fw/multicast-strategy.cpp
index 244ccfc..ee921c3 100644
--- a/daemon/fw/multicast-strategy.cpp
+++ b/daemon/fw/multicast-strategy.cpp
@@ -69,10 +69,6 @@
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 @@
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 @@
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 526d7b0..6871821 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 @@
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> interest = makeInterest("ndn:/H0D6i5fc");
+ shared_ptr<pit::Entry> 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<MulticastStrategy>(node);
+ }
+
+ shared_ptr<TopologyLink> linkAB = topo.addLink("AB", 10_ms, {nodeA, nodeB}),
+ linkBC = topo.addLink("BC", 100_ms, {nodeB, nodeC});
+
+ shared_ptr<TopologyAppLink> 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;