fw: handle dropped packets in strategies

refs #5128

Change-Id: Ic7f7e61b2dde66004d0291bb41e008961fc7b252

daemon/fw/access-strategy.cpp

@@ -144,7 +144,9 @@ AccessStrategy::sendToLastNexthop(const FaceEndpoint& ingress, const Interest& i
   NFD_LOG_DEBUG(pitEntry->getInterest() << " interestTo " << mi.lastNexthop
                 << " last-nexthop rto=" << time::duration_cast<time::microseconds>(rto).count());
 
-  this->sendInterest(pitEntry, *outFace, interest);
+  if (!this->sendInterest(pitEntry, *outFace, interest)) {
+    return false;
+  }
 
   // schedule RTO timeout
   PitInfo* pi = pitEntry->insertStrategyInfo<PitInfo>().first;
@@ -198,8 +200,9 @@ AccessStrategy::multicast(const Face& inFace, const Interest& interest,
       continue;
     }
     NFD_LOG_DEBUG(pitEntry->getInterest() << " interestTo " << outFace.getId() << " multicast");
-    this->sendInterest(pitEntry, outFace, interest);
-    ++nSent;
+    if (this->sendInterest(pitEntry, outFace, interest)) {
+      ++nSent;
+    }
   }
   return nSent;
 }

daemon/fw/algorithm.hpp

@@ -23,8 +23,8 @@
  * NFD, e.g., in COPYING.md file.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef NFD_DAEMON_FW_PIT_ALGORITHM_HPP
-#define NFD_DAEMON_FW_PIT_ALGORITHM_HPP
+#ifndef NFD_DAEMON_FW_ALGORITHM_HPP
+#define NFD_DAEMON_FW_ALGORITHM_HPP
 
 #include "fw/scope-prefix.hpp"
 #include "table/fib.hpp"
@@ -110,4 +110,4 @@ isNextHopEligible(const Face& inFace, const Interest& interest,
 } // namespace fw
 } // namespace nfd
 
-#endif // NFD_DAEMON_FW_PIT_ALGORITHM_HPP
+#endif // NFD_DAEMON_FW_ALGORITHM_HPP

daemon/fw/forwarder.cpp

@@ -240,7 +240,7 @@ Forwarder::onContentStoreHit(const FaceEndpoint& ingress, const shared_ptr<pit::
     [&] (fw::Strategy& strategy) { strategy.afterContentStoreHit(pitEntry, ingress, data); });
 }
 
-void
+pit::OutRecord*
 Forwarder::onOutgoingInterest(const shared_ptr<pit::Entry>& pitEntry,
                               Face& egress, const Interest& interest)
 {
@@ -249,17 +249,19 @@ Forwarder::onOutgoingInterest(const shared_ptr<pit::Entry>& pitEntry,
     NFD_LOG_DEBUG("onOutgoingInterest out=" << egress.getId() << " interest=" << pitEntry->getName()
                   << " non-local hop-limit=0");
     ++const_cast<PacketCounter&>(egress.getCounters().nOutHopLimitZero);
-    return;
+    return nullptr;
   }
 
   NFD_LOG_DEBUG("onOutgoingInterest out=" << egress.getId() << " interest=" << pitEntry->getName());
 
   // insert out-record
-  pitEntry->insertOrUpdateOutRecord(egress, interest);
+  auto it = pitEntry->insertOrUpdateOutRecord(egress, interest);
+  BOOST_ASSERT(it != pitEntry->out_end());
 
   // send Interest
   egress.sendInterest(interest);
   ++m_counters.nOutInterests;
+  return &*it;
 }
 
 void
@@ -397,12 +399,12 @@ Forwarder::onDataUnsolicited(const FaceEndpoint& ingress, const Data& data)
   ++m_counters.nUnsolicitedData;
 }
 
-void
+bool
 Forwarder::onOutgoingData(const Data& data, Face& egress)
 {
   if (egress.getId() == face::INVALID_FACEID) {
     NFD_LOG_WARN("onOutgoingData out=(invalid) data=" << data.getName());
-    return;
+    return false;
   }
   NFD_LOG_DEBUG("onOutgoingData out=" << egress.getId() << " data=" << data.getName());
 
@@ -413,7 +415,7 @@ Forwarder::onOutgoingData(const Data& data, Face& egress)
     NFD_LOG_DEBUG("onOutgoingData out=" << egress.getId() << " data=" << data.getName()
                   << " violates /localhost");
     // (drop)
-    return;
+    return false;
   }
 
   // TODO traffic manager
@@ -421,6 +423,8 @@ Forwarder::onOutgoingData(const Data& data, Face& egress)
   // send Data
   egress.sendData(data);
   ++m_counters.nOutData;
+
+  return true;
 }
 
 void
@@ -480,14 +484,14 @@ Forwarder::onIncomingNack(const FaceEndpoint& ingress, const lp::Nack& nack)
     [&] (fw::Strategy& strategy) { strategy.afterReceiveNack(ingress, nack, pitEntry); });
 }
 
-void
+bool
 Forwarder::onOutgoingNack(const shared_ptr<pit::Entry>& pitEntry,
                           Face& egress, const lp::NackHeader& nack)
 {
   if (egress.getId() == face::INVALID_FACEID) {
     NFD_LOG_WARN("onOutgoingNack out=(invalid)"
                  << " nack=" << pitEntry->getInterest().getName() << "~" << nack.getReason());
-    return;
+    return false;
   }
 
   // has in-record?
@@ -498,7 +502,7 @@ Forwarder::onOutgoingNack(const shared_ptr<pit::Entry>& pitEntry,
     NFD_LOG_DEBUG("onOutgoingNack out=" << egress.getId()
                   << " nack=" << pitEntry->getInterest().getName()
                   << "~" << nack.getReason() << " no-in-record");
-    return;
+    return false;
   }
 
   // if multi-access or ad hoc face, drop
@@ -506,7 +510,7 @@ Forwarder::onOutgoingNack(const shared_ptr<pit::Entry>& pitEntry,
     NFD_LOG_DEBUG("onOutgoingNack out=" << egress.getId()
                   << " nack=" << pitEntry->getInterest().getName() << "~" << nack.getReason()
                   << " link-type=" << egress.getLinkType());
-    return;
+    return false;
   }
 
   NFD_LOG_DEBUG("onOutgoingNack out=" << egress.getId()
@@ -523,6 +527,8 @@ Forwarder::onOutgoingNack(const shared_ptr<pit::Entry>& pitEntry,
   // send Nack on face
   egress.sendNack(nackPkt);
   ++m_counters.nOutNacks;
+
+  return true;
 }
 
 void

daemon/fw/forwarder.hpp

@@ -189,8 +189,9 @@ PUBLIC_WITH_TESTS_ELSE_PRIVATE: // pipelines
                     const Interest& interest, const Data& data);
 
   /** \brief outgoing Interest pipeline
+   *  \return A pointer to the out-record created or nullptr if the Interest was dropped
    */
-  VIRTUAL_WITH_TESTS void
+  VIRTUAL_WITH_TESTS pit::OutRecord*
   onOutgoingInterest(const shared_ptr<pit::Entry>& pitEntry,
                      Face& egress, const Interest& interest);
 
@@ -210,8 +211,9 @@ PUBLIC_WITH_TESTS_ELSE_PRIVATE: // pipelines
   onDataUnsolicited(const FaceEndpoint& ingress, const Data& data);
 
   /** \brief outgoing Data pipeline
+   *  \return Whether the Data was transmitted (true) or dropped (false)
    */
-  VIRTUAL_WITH_TESTS void
+  VIRTUAL_WITH_TESTS bool
   onOutgoingData(const Data& data, Face& egress);
 
   /** \brief incoming Nack pipeline
@@ -220,8 +222,9 @@ PUBLIC_WITH_TESTS_ELSE_PRIVATE: // pipelines
   onIncomingNack(const FaceEndpoint& ingress, const lp::Nack& nack);
 
   /** \brief outgoing Nack pipeline
+   *  \return Whether the Nack was transmitted (true) or dropped (false)
    */
-  VIRTUAL_WITH_TESTS void
+  VIRTUAL_WITH_TESTS bool
   onOutgoingNack(const shared_ptr<pit::Entry>& pitEntry,
                  Face& egress, const lp::NackHeader& nack);
 

daemon/fw/multicast-strategy.cpp

@@ -83,10 +83,9 @@ MulticastStrategy::afterReceiveInterest(const FaceEndpoint& ingress, const Inter
       continue;
     }
 
-    this->sendInterest(pitEntry, outFace, interest);
     NFD_LOG_DEBUG(interest << " from=" << ingress << " pitEntry-to=" << outFace.getId());
-
-    if (suppressResult == RetxSuppressionResult::FORWARD) {
+    bool wasSent = this->sendInterest(pitEntry, outFace, interest) != nullptr;
+    if (wasSent && suppressResult == RetxSuppressionResult::FORWARD) {
       m_retxSuppression.incrementIntervalForOutRecord(*pitEntry->getOutRecord(outFace));
     }
   }

daemon/fw/self-learning-strategy.cpp

@@ -149,13 +149,17 @@ SelfLearningStrategy::broadcastInterest(const Interest& interest, const Face& in
 {
   for (auto& outFace : this->getFaceTable() | boost::adaptors::reversed) {
     if ((outFace.getId() == inFace.getId() && outFace.getLinkType() != ndn::nfd::LINK_TYPE_AD_HOC) ||
-        wouldViolateScope(inFace, interest, outFace) || outFace.getScope() == ndn::nfd::FACE_SCOPE_LOCAL) {
+        wouldViolateScope(inFace, interest, outFace) ||
+        outFace.getScope() == ndn::nfd::FACE_SCOPE_LOCAL) {
       continue;
     }
-    this->sendInterest(pitEntry, outFace, interest);
-    pitEntry->getOutRecord(outFace)->insertStrategyInfo<OutRecordInfo>().first->isNonDiscoveryInterest = false;
-    NFD_LOG_DEBUG("send discovery Interest=" << interest << " from="
-                  << inFace.getId() << " to=" << outFace.getId());
+
+    NFD_LOG_DEBUG("send discovery Interest=" << interest << " from=" << inFace.getId() <<
+                  " to=" << outFace.getId());
+    auto outRecord = this->sendInterest(pitEntry, outFace, interest);
+    if (outRecord != nullptr) {
+      outRecord->insertStrategyInfo<OutRecordInfo>().first->isNonDiscoveryInterest = false;
+    }
   }
 }
 
@@ -165,15 +169,17 @@ SelfLearningStrategy::multicastInterest(const Interest& interest, const Face& in
                                         const fib::NextHopList& nexthops)
 {
   for (const auto& nexthop : nexthops) {
-    Face& outFace = nexthop.getFace();
-    if ((outFace.getId() == inFace.getId() && outFace.getLinkType() != ndn::nfd::LINK_TYPE_AD_HOC) ||
-        wouldViolateScope(inFace, interest, outFace)) {
+    if (!isNextHopEligible(inFace, interest, nexthop, pitEntry)) {
       continue;
     }
-    this->sendInterest(pitEntry, outFace, interest);
-    pitEntry->getOutRecord(outFace)->insertStrategyInfo<OutRecordInfo>().first->isNonDiscoveryInterest = true;
-    NFD_LOG_DEBUG("send non-discovery Interest=" << interest << " from="
-                  << inFace.getId() << " to=" << outFace.getId());
+
+    Face& outFace = nexthop.getFace();
+    NFD_LOG_DEBUG("send non-discovery Interest=" << interest << " from=" << inFace.getId() <<
+                  " to=" << outFace.getId());
+    auto outRecord = this->sendInterest(pitEntry, outFace, interest);
+    if (outRecord != nullptr) {
+      outRecord->insertStrategyInfo<OutRecordInfo>().first->isNonDiscoveryInterest = true;
+    }
   }
 }
 

daemon/fw/strategy.cpp

@@ -193,16 +193,15 @@ Strategy::onDroppedInterest(const Face& egress, const Interest& interest)
   NFD_LOG_DEBUG("onDroppedInterest out=" << egress.getId() << " name=" << interest.getName());
 }
 
-void
+pit::OutRecord*
 Strategy::sendInterest(const shared_ptr<pit::Entry>& pitEntry, Face& egress, const Interest& interest)
 {
   if (interest.getTag<lp::PitToken>() != nullptr) {
     Interest interest2 = interest; // make a copy to preserve tag on original packet
     interest2.removeTag<lp::PitToken>();
-    m_forwarder.onOutgoingInterest(pitEntry, egress, interest2);
-    return;
+    return m_forwarder.onOutgoingInterest(pitEntry, egress, interest2);
   }
-  m_forwarder.onOutgoingInterest(pitEntry, egress, interest);
+  return m_forwarder.onOutgoingInterest(pitEntry, egress, interest);
 }
 
 void
@@ -212,7 +211,7 @@ Strategy::afterNewNextHop(const fib::NextHop& nextHop, const shared_ptr<pit::Ent
                 << " nexthop=" << nextHop.getFace().getId());
 }
 
-void
+bool
 Strategy::sendData(const shared_ptr<pit::Entry>& pitEntry, const Data& data, Face& egress)
 {
   BOOST_ASSERT(pitEntry->getInterest().matchesData(data));
@@ -230,10 +229,9 @@ Strategy::sendData(const shared_ptr<pit::Entry>& pitEntry, const Data& data, Fac
   if (pitToken != nullptr) {
     Data data2 = data; // make a copy so each downstream can get a different PIT token
     data2.setTag(pitToken);
-    m_forwarder.onOutgoingData(data2, egress);
-    return;
+    return m_forwarder.onOutgoingData(data2, egress);
   }
-  m_forwarder.onOutgoingData(data, egress);
+  return m_forwarder.onOutgoingData(data, egress);
 }
 
 void

daemon/fw/strategy.hpp

@@ -255,8 +255,9 @@ protected: // actions
    *  \param pitEntry the PIT entry
    *  \param egress face through which to send out the Interest
    *  \param interest the Interest packet
+   *  \return A pointer to the out-record created or nullptr if the Interest was dropped
    */
-  VIRTUAL_WITH_TESTS void
+  VIRTUAL_WITH_TESTS pit::OutRecord*
   sendInterest(const shared_ptr<pit::Entry>& pitEntry, Face& egress,
                const Interest& interest);
 
@@ -264,8 +265,9 @@ protected: // actions
    *  \param pitEntry the PIT entry
    *  \param data the Data packet
    *  \param egress face through which to send out the Data
+   *  \return Whether the Data was sent (true) or dropped (false)
    */
-  VIRTUAL_WITH_TESTS void
+  VIRTUAL_WITH_TESTS bool
   sendData(const shared_ptr<pit::Entry>& pitEntry, const Data& data, Face& egress);
 
   /** \brief Send a Data packet to all matched and qualified faces.
@@ -298,12 +300,13 @@ protected: // actions
    *  \param pitEntry the PIT entry
    *  \param egress face through which to send out the Nack
    *  \param header the Nack header
+   *  \return Whether the Nack was sent (true) or dropped (false)
    */
-  VIRTUAL_WITH_TESTS void
+  VIRTUAL_WITH_TESTS bool
   sendNack(const shared_ptr<pit::Entry>& pitEntry, Face& egress,
            const lp::NackHeader& header)
   {
-    m_forwarder.onOutgoingNack(pitEntry, egress, header);
+    return m_forwarder.onOutgoingNack(pitEntry, egress, header);
   }
 
   /** \brief Send Nack to every face that has an in-record, except those in \p exceptFaces

tests/daemon/fw/forwarder.t.cpp

@@ -161,16 +161,20 @@ BOOST_AUTO_TEST_CASE(OutgoingInterest)
 
   Pit& pit = forwarder.getPit();
   auto interestA1 = makeInterest("/A", false, nullopt, 8378);
-  shared_ptr<pit::Entry> pitA = pit.insert(*interestA1).first;
+  auto pitA = pit.insert(*interestA1).first;
   pitA->insertOrUpdateInRecord(*face1, *interestA1);
 
   auto interestA2 = makeInterest("/A", false, nullopt, 1698);
-  forwarder.onOutgoingInterest(pitA, *face2, *interestA2);
-
-  auto outA2 = pitA->getOutRecord(*face2);
-  BOOST_REQUIRE(outA2 != pitA->out_end());
+  auto outA2 = forwarder.onOutgoingInterest(pitA, *face2, *interestA2);
+  BOOST_REQUIRE(outA2 != nullptr);
   BOOST_CHECK_EQUAL(outA2->getLastNonce(), 1698);
 
+  // This packet will be dropped because HopLimit=0
+  auto interestA3 = makeInterest("/A", false, nullopt, 9876);
+  interestA3->setHopLimit(0);
+  auto outA3 = forwarder.onOutgoingInterest(pitA, *face2, *interestA3);
+  BOOST_CHECK(outA3 == nullptr);
+
   BOOST_REQUIRE_EQUAL(face2->sentInterests.size(), 1);
   BOOST_CHECK_EQUAL(face2->sentInterests.back().getNonce(), 1698);
 }
@@ -339,8 +343,8 @@ BOOST_AUTO_TEST_CASE(IncomingInterestStrategyDispatch)
   auto face1 = addFace();
   auto face2 = addFace();
 
-  DummyStrategy& strategyA = choose<DummyStrategy>(forwarder, "/", DummyStrategy::getStrategyName());
-  DummyStrategy& strategyB = choose<DummyStrategy>(forwarder, "/B", DummyStrategy::getStrategyName());
+  auto& strategyA = choose<DummyStrategy>(forwarder, "/", DummyStrategy::getStrategyName());
+  auto& strategyB = choose<DummyStrategy>(forwarder, "/B", DummyStrategy::getStrategyName());
 
   auto interest1 = makeInterest("/A/1");
   strategyA.afterReceiveInterest_count = 0;
@@ -381,14 +385,14 @@ BOOST_AUTO_TEST_CASE(IncomingData)
 
   Pit& pit = forwarder.getPit();
   auto interestD = makeInterest("/A/B/C/D");
-  shared_ptr<pit::Entry> pitD = pit.insert(*interestD).first;
+  auto pitD = pit.insert(*interestD).first;
   pitD->insertOrUpdateInRecord(*face1, *interestD);
   auto interestA = makeInterest("/A", true);
-  shared_ptr<pit::Entry> pitA = pit.insert(*interestA).first;
+  auto pitA = pit.insert(*interestA).first;
   pitA->insertOrUpdateInRecord(*face2, *interestA);
   pitA->insertOrUpdateInRecord(*face3, *interestA);
   auto interestC = makeInterest("/A/B/C", true);
-  shared_ptr<pit::Entry> pitC = pit.insert(*interestC).first;
+  auto pitC = pit.insert(*interestC).first;
   pitC->insertOrUpdateInRecord(*face3, *interestC);
   pitC->insertOrUpdateInRecord(*face4, *interestC);
 
@@ -406,6 +410,36 @@ BOOST_AUTO_TEST_CASE(IncomingData)
   BOOST_CHECK_EQUAL(forwarder.getCounters().nUnsolicitedData, 0);
 }
 
+BOOST_AUTO_TEST_CASE(OutgoingData)
+{
+  auto face1 = addFace("dummy://", "dummy://", ndn::nfd::FACE_SCOPE_LOCAL);
+  auto face2 = addFace("dummy://", "dummy://", ndn::nfd::FACE_SCOPE_NON_LOCAL);
+  auto face3 = addFace();
+  face3->setId(face::INVALID_FACEID);
+
+  auto data = makeData("/QkzAWU6K");
+  auto localData = makeData("/localhost/YH8bqnbv");
+
+  face1->sentData.clear();
+  BOOST_CHECK(forwarder.onOutgoingData(*data, *face1));
+  BOOST_REQUIRE_EQUAL(face1->sentData.size(), 1);
+  BOOST_CHECK_EQUAL(face1->sentData.back().getName(), data->getName());
+
+  // scope control
+  face1->sentData.clear();
+  face2->sentData.clear();
+  BOOST_CHECK(!forwarder.onOutgoingData(*localData, *face2));
+  BOOST_CHECK_EQUAL(face2->sentData.size(), 0);
+  BOOST_CHECK(forwarder.onOutgoingData(*localData, *face1));
+  BOOST_REQUIRE_EQUAL(face1->sentData.size(), 1);
+  BOOST_CHECK_EQUAL(face1->sentData.back().getName(), localData->getName());
+
+  // face with invalid ID
+  face3->sentData.clear();
+  BOOST_CHECK(!forwarder.onOutgoingData(*data, *face3));
+  BOOST_CHECK_EQUAL(face3->sentData.size(), 0);
+}
+
 BOOST_AUTO_TEST_CASE(IncomingNack)
 {
   auto face1 = addFace();
@@ -415,27 +449,27 @@ BOOST_AUTO_TEST_CASE(IncomingNack)
                        ndn::nfd::FACE_PERSISTENCY_PERSISTENT,
                        ndn::nfd::LINK_TYPE_MULTI_ACCESS);
 
-  DummyStrategy& strategyA = choose<DummyStrategy>(forwarder, "/", DummyStrategy::getStrategyName());
-  DummyStrategy& strategyB = choose<DummyStrategy>(forwarder, "/B", DummyStrategy::getStrategyName());
+  auto& strategyA = choose<DummyStrategy>(forwarder, "/", DummyStrategy::getStrategyName());
+  auto& strategyB = choose<DummyStrategy>(forwarder, "/B", DummyStrategy::getStrategyName());
 
   Pit& pit = forwarder.getPit();
 
   // dispatch to the correct strategy
   auto interest1 = makeInterest("/A/AYJqayrzF", false, nullopt, 562);
-  shared_ptr<pit::Entry> pit1 = pit.insert(*interest1).first;
+  auto pit1 = pit.insert(*interest1).first;
   pit1->insertOrUpdateOutRecord(*face1, *interest1);
   auto interest2 = makeInterest("/B/EVyP73ru", false, nullopt, 221);
-  shared_ptr<pit::Entry> pit2 = pit.insert(*interest2).first;
+  auto pit2 = pit.insert(*interest2).first;
   pit2->insertOrUpdateOutRecord(*face1, *interest2);
 
-  lp::Nack nack1 = makeNack(*interest1, lp::NackReason::CONGESTION);
+  auto nack1 = makeNack(*interest1, lp::NackReason::CONGESTION);
   strategyA.afterReceiveNack_count = 0;
   strategyB.afterReceiveNack_count = 0;
   forwarder.onIncomingNack(FaceEndpoint(*face1, 0), nack1);
   BOOST_CHECK_EQUAL(strategyA.afterReceiveNack_count, 1);
   BOOST_CHECK_EQUAL(strategyB.afterReceiveNack_count, 0);
 
-  lp::Nack nack2 = makeNack(*interest2, lp::NackReason::CONGESTION);
+  auto nack2 = makeNack(*interest2, lp::NackReason::CONGESTION);
   strategyA.afterReceiveNack_count = 0;
   strategyB.afterReceiveNack_count = 0;
   forwarder.onIncomingNack(FaceEndpoint(*face1, 0), nack2);
@@ -449,7 +483,7 @@ BOOST_AUTO_TEST_CASE(IncomingNack)
   BOOST_CHECK_EQUAL(outRecord1->getIncomingNack()->getReason(), lp::NackReason::CONGESTION);
 
   // drop if no PIT entry
-  lp::Nack nack3 = makeNack(*makeInterest("/yEcw5HhdM", false, nullopt, 243), lp::NackReason::CONGESTION);
+  auto nack3 = makeNack(*makeInterest("/yEcw5HhdM", false, nullopt, 243), lp::NackReason::CONGESTION);
   strategyA.afterReceiveNack_count = 0;
   strategyB.afterReceiveNack_count = 0;
   forwarder.onIncomingNack(FaceEndpoint(*face1, 0), nack3);
@@ -458,10 +492,10 @@ BOOST_AUTO_TEST_CASE(IncomingNack)
 
   // drop if no out-record
   auto interest4 = makeInterest("/Etab4KpY", false, nullopt, 157);
-  shared_ptr<pit::Entry> pit4 = pit.insert(*interest4).first;
+  auto pit4 = pit.insert(*interest4).first;
   pit4->insertOrUpdateOutRecord(*face1, *interest4);
 
-  lp::Nack nack4a = makeNack(*interest4, lp::NackReason::CONGESTION);
+  auto nack4a = makeNack(*interest4, lp::NackReason::CONGESTION);
   strategyA.afterReceiveNack_count = 0;
   strategyB.afterReceiveNack_count = 0;
   forwarder.onIncomingNack(FaceEndpoint(*face2, 0), nack4a);
@@ -469,7 +503,7 @@ BOOST_AUTO_TEST_CASE(IncomingNack)
   BOOST_CHECK_EQUAL(strategyB.afterReceiveNack_count, 0);
 
   // drop if Nonce does not match out-record
-  lp::Nack nack4b = makeNack(*makeInterest("/Etab4KpY", false, nullopt, 294), lp::NackReason::CONGESTION);
+  auto nack4b = makeNack(*makeInterest("/Etab4KpY", false, nullopt, 294), lp::NackReason::CONGESTION);
   strategyA.afterReceiveNack_count = 0;
   strategyB.afterReceiveNack_count = 0;
   forwarder.onIncomingNack(FaceEndpoint(*face1, 0), nack4b);
@@ -493,6 +527,8 @@ BOOST_AUTO_TEST_CASE(OutgoingNack)
                        ndn::nfd::FACE_SCOPE_NON_LOCAL,
                        ndn::nfd::FACE_PERSISTENCY_PERSISTENT,
                        ndn::nfd::LINK_TYPE_MULTI_ACCESS);
+  auto face4 = addFace();
+  face4->setId(face::INVALID_FACEID);
 
   Pit& pit = forwarder.getPit();
 
@@ -501,39 +537,41 @@ BOOST_AUTO_TEST_CASE(OutgoingNack)
 
   // don't send Nack if there's no in-record
   auto interest1 = makeInterest("/fM5IVEtC", false, nullopt, 719);
-  shared_ptr<pit::Entry> pit1 = pit.insert(*interest1).first;
+  auto pit1 = pit.insert(*interest1).first;
   pit1->insertOrUpdateInRecord(*face1, *interest1);
 
   face2->sentNacks.clear();
-  forwarder.onOutgoingNack(pit1, *face2, nackHeader);
+  BOOST_CHECK(!forwarder.onOutgoingNack(pit1, *face2, nackHeader));
   BOOST_CHECK_EQUAL(face2->sentNacks.size(), 0);
 
   // send Nack with correct Nonce
   auto interest2a = makeInterest("/Vi8tRm9MG3", false, nullopt, 152);
-  shared_ptr<pit::Entry> pit2 = pit.insert(*interest2a).first;
+  auto pit2 = pit.insert(*interest2a).first;
   pit2->insertOrUpdateInRecord(*face1, *interest2a);
   auto interest2b = makeInterest("/Vi8tRm9MG3", false, nullopt, 808);
   pit2->insertOrUpdateInRecord(*face2, *interest2b);
-
   face1->sentNacks.clear();
-  forwarder.onOutgoingNack(pit2, *face1, nackHeader);
+  face2->sentNacks.clear();
+
+  BOOST_CHECK(forwarder.onOutgoingNack(pit2, *face1, nackHeader));
   BOOST_REQUIRE_EQUAL(face1->sentNacks.size(), 1);
   BOOST_CHECK_EQUAL(face1->sentNacks.back().getReason(), lp::NackReason::CONGESTION);
   BOOST_CHECK_EQUAL(face1->sentNacks.back().getInterest().getNonce(), 152);
+  BOOST_CHECK_EQUAL(face2->sentNacks.size(), 0);
 
-  // erase in-record
+  // in-record is erased
   auto inRecord2a = pit2->getInRecord(*face1);
   BOOST_CHECK(inRecord2a == pit2->in_end());
 
   // send Nack with correct Nonce
-  face2->sentNacks.clear();
-  forwarder.onOutgoingNack(pit2, *face2, nackHeader);
+  BOOST_CHECK(forwarder.onOutgoingNack(pit2, *face2, nackHeader));
+  BOOST_CHECK_EQUAL(face1->sentNacks.size(), 1);
   BOOST_REQUIRE_EQUAL(face2->sentNacks.size(), 1);
   BOOST_CHECK_EQUAL(face2->sentNacks.back().getReason(), lp::NackReason::CONGESTION);
   BOOST_CHECK_EQUAL(face2->sentNacks.back().getInterest().getNonce(), 808);
 
-  // erase in-record
-  auto inRecord2b = pit2->getInRecord(*face1);
+  // in-record is erased
+  auto inRecord2b = pit2->getInRecord(*face2);
   BOOST_CHECK(inRecord2b == pit2->in_end());
 
   // don't send Nack to multi-access face
@@ -541,8 +579,16 @@ BOOST_AUTO_TEST_CASE(OutgoingNack)
   pit2->insertOrUpdateInRecord(*face3, *interest2c);
 
   face3->sentNacks.clear();
-  forwarder.onOutgoingNack(pit1, *face3, nackHeader);
+  BOOST_CHECK(!forwarder.onOutgoingNack(pit2, *face3, nackHeader));
   BOOST_CHECK_EQUAL(face3->sentNacks.size(), 0);
+
+  // don't send Nack to face with invalid ID
+  auto interest1b = makeInterest("/fM5IVEtC", false, nullopt, 553);
+  pit1->insertOrUpdateInRecord(*face4, *interest1b);
+
+  face4->sentNacks.clear();
+  BOOST_CHECK(!forwarder.onOutgoingNack(pit1, *face4, nackHeader));
+  BOOST_CHECK_EQUAL(face4->sentNacks.size(), 0);
 }
 
 BOOST_AUTO_TEST_CASE(InterestLoopNack)

tests/daemon/fw/strategy-tester.hpp

@@ -107,13 +107,15 @@ public:
   }
 
 protected:
-  void
+  pit::OutRecord*
   sendInterest(const shared_ptr<pit::Entry>& pitEntry, Face& egress,
                const Interest& interest) override
   {
     sendInterestHistory.push_back({pitEntry->getInterest(), egress.getId(), interest});
-    pitEntry->insertOrUpdateOutRecord(egress, interest);
+    auto it = pitEntry->insertOrUpdateOutRecord(egress, interest);
+    BOOST_ASSERT(it != pitEntry->out_end());
     afterAction();
+    return &*it;
   }
 
   void
@@ -123,13 +125,14 @@ protected:
     afterAction();
   }
 
-  void
+  bool
   sendNack(const shared_ptr<pit::Entry>& pitEntry, Face& egress,
            const lp::NackHeader& header) override
   {
     sendNackHistory.push_back({pitEntry->getInterest(), egress.getId(), header});
     pitEntry->deleteInRecord(egress);
     afterAction();
+    return true;
   }
 
 public:

tests/daemon/fw/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,
@@ -43,7 +43,7 @@ using namespace nfd::tests;
 BOOST_AUTO_TEST_SUITE(Fw)
 BOOST_FIXTURE_TEST_SUITE(TestStrategy, GlobalIoFixture)
 
-// Strategy registry is tested in table/strategy-choice.t.cpp and strategy-instantiation.t.cpp
+// Strategy registry is tested in strategy-choice.t.cpp and strategy-instantiation.t.cpp
 
 class FaceTableAccessTestStrategy : public DummyStrategy
 {
@@ -103,8 +103,6 @@ BOOST_AUTO_TEST_CASE(FaceTableAccess)
   BOOST_CHECK((strategy.removedFaces == std::vector<FaceId>{id2, id1}));
 }
 
-// LookupFib is tested in Fw/TestLinkForwarding test suite.
-
 BOOST_AUTO_TEST_SUITE_END() // TestStrategy
 BOOST_AUTO_TEST_SUITE_END() // Fw