fw: delegate sending Nack-Duplicate to forwarding strategy

This commit introduces a new strategy trigger (onInterestLoop) that
is invoked when a duplicate Interest is received. The default behavior
of the new trigger is to send a Nack as before. We also modify the
MulticastStrategy to override onInterestLoop and skip sending the Nack.

Refs: #5278
Co-authored-by: Davide Pesavento <davidepesa@gmail.com>
Change-Id: I07bb520bd31cbfc8d8f8581e05c49927f8ea35f7

daemon/fw/forwarder.cpp

@@ -1,6 +1,6 @@
 /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
 /*
- * Copyright (c) 2014-2023,  Regents of the University of California,
+ * Copyright (c) 2014-2024,  Regents of the University of California,
  *                           Arizona Board of Regents,
  *                           Colorado State University,
  *                           University Pierre & Marie Curie, Sorbonne University,
@@ -125,7 +125,7 @@ Forwarder::onIncomingInterest(const Interest& interest, const FaceEndpoint& ingr
   // detect duplicate Nonce with Dead Nonce List
   bool hasDuplicateNonceInDnl = m_deadNonceList.has(interest.getName(), nonce);
   if (hasDuplicateNonceInDnl) {
-    // goto Interest loop pipeline
+    // go to Interest loop pipeline
     this->onInterestLoop(interest, ingress);
     return;
   }
@@ -149,7 +149,7 @@ Forwarder::onIncomingInterest(const Interest& interest, const FaceEndpoint& ingr
     hasDuplicateNonceInPit = hasDuplicateNonceInPit && !(dnw & fw::DUPLICATE_NONCE_IN_SAME);
   }
   if (hasDuplicateNonceInPit) {
-    // goto Interest loop pipeline
+    // go to Interest loop pipeline
     this->onInterestLoop(interest, ingress);
     return;
   }
@@ -176,14 +176,10 @@ Forwarder::onInterestLoop(const Interest& interest, const FaceEndpoint& ingress)
   }
 
   NFD_LOG_DEBUG("onInterestLoop in=" << ingress << " interest=" << interest.getName()
-                << " nonce=" << interest.getNonce() << " nack");
+                << " nonce=" << interest.getNonce());
 
-  // send Nack with reason=DUPLICATE
-  // note: Don't enter outgoing Nack pipeline because it needs an in-record.
-  lp::Nack nack(interest);
-  nack.setReason(lp::NackReason::DUPLICATE);
-  ingress.face.sendNack(nack);
-  ++m_counters.nOutNacks;
+  // leave loop handling up to the strategy (e.g., whether to reply with a Nack)
+  m_strategyChoice.findEffectiveStrategy(interest.getName()).onInterestLoop(interest, ingress);
 }
 
 void
@@ -317,7 +313,7 @@ Forwarder::onIncomingData(const Data& data, const FaceEndpoint& ingress)
   // PIT match
   pit::DataMatchResult pitMatches = m_pit.findAllDataMatches(data);
   if (pitMatches.size() == 0) {
-    // goto Data unsolicited pipeline
+    // go to Data unsolicited pipeline
     this->onDataUnsolicited(data, ingress);
     return;
   }
@@ -387,7 +383,7 @@ Forwarder::onIncomingData(const Data& data, const FaceEndpoint& ingress)
           pendingDownstream->getLinkType() != ndn::nfd::LINK_TYPE_AD_HOC) {
         continue;
       }
-      // goto outgoing Data pipeline
+      // go to outgoing Data pipeline
       this->onOutgoingData(data, *pendingDownstream);
     }
   }
@@ -415,7 +411,6 @@ Forwarder::onOutgoingData(const Data& data, Face& egress)
     NFD_LOG_WARN("onOutgoingData out=(invalid) data=" << data.getName());
     return false;
   }
-  NFD_LOG_DEBUG("onOutgoingData out=" << egress.getId() << " data=" << data.getName());
 
   // /localhost scope control
   bool isViolatingLocalhost = egress.getScope() == ndn::nfd::FACE_SCOPE_NON_LOCAL &&
@@ -427,7 +422,7 @@ Forwarder::onOutgoingData(const Data& data, Face& egress)
     return false;
   }
 
-  // TODO traffic manager
+  NFD_LOG_DEBUG("onOutgoingData out=" << egress.getId() << " data=" << data.getName());
 
   // send Data
   egress.sendData(data);
@@ -486,7 +481,7 @@ Forwarder::onIncomingNack(const lp::Nack& nack, const FaceEndpoint& ingress)
     this->setExpiryTimer(pitEntry, 0_ms);
   }
 
-  // trigger strategy: after receive NACK
+  // trigger strategy: after receive Nack
   m_strategyChoice.findEffectiveStrategy(*pitEntry).afterReceiveNack(nack, ingress, pitEntry);
 }
 
@@ -495,8 +490,8 @@ Forwarder::onOutgoingNack(const lp::NackHeader& nack, Face& egress,
                           const shared_ptr<pit::Entry>& pitEntry)
 {
   if (egress.getId() == face::INVALID_FACEID) {
-    NFD_LOG_WARN("onOutgoingNack out=(invalid)"
-                 << " nack=" << pitEntry->getInterest().getName() << "~" << nack.getReason());
+    NFD_LOG_WARN("onOutgoingNack out=(invalid)" << " nack=" << pitEntry->getName()
+                 << "~" << nack.getReason());
     return false;
   }
 
@@ -505,23 +500,20 @@ Forwarder::onOutgoingNack(const lp::NackHeader& nack, Face& egress,
 
   // if no in-record found, drop
   if (inRecord == pitEntry->in_end()) {
-    NFD_LOG_DEBUG("onOutgoingNack out=" << egress.getId()
-                  << " nack=" << pitEntry->getInterest().getName()
+    NFD_LOG_DEBUG("onOutgoingNack out=" << egress.getId() << " nack=" << pitEntry->getName()
                   << "~" << nack.getReason() << " no-in-record");
     return false;
   }
 
   // if multi-access or ad hoc face, drop
   if (egress.getLinkType() != ndn::nfd::LINK_TYPE_POINT_TO_POINT) {
-    NFD_LOG_DEBUG("onOutgoingNack out=" << egress.getId()
-                  << " nack=" << pitEntry->getInterest().getName() << "~" << nack.getReason()
-                  << " link-type=" << egress.getLinkType());
+    NFD_LOG_DEBUG("onOutgoingNack out=" << egress.getId() << " nack=" << pitEntry->getName()
+                  << "~" << nack.getReason() << " link-type=" << egress.getLinkType());
     return false;
   }
 
-  NFD_LOG_DEBUG("onOutgoingNack out=" << egress.getId()
-                << " nack=" << pitEntry->getInterest().getName()
-                << "~" << nack.getReason() << " OK");
+  NFD_LOG_DEBUG("onOutgoingNack out=" << egress.getId() << " nack=" << pitEntry->getName()
+                << "~" << nack.getReason());
 
   // create Nack packet with the Interest from in-record
   lp::Nack nackPkt(inRecord->getInterest());

daemon/fw/multicast-strategy.cpp

@@ -53,7 +53,7 @@ MulticastStrategy::MulticastStrategy(Forwarder& forwarder, const Name& name)
 const Name&
 MulticastStrategy::getStrategyName()
 {
-  static const auto strategyName = Name("/localhost/nfd/strategy/multicast").appendVersion(4);
+  static const auto strategyName = Name("/localhost/nfd/strategy/multicast").appendVersion(5);
   return strategyName;
 }
 

daemon/fw/multicast-strategy.hpp

@@ -1,6 +1,6 @@
 /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
 /*
- * Copyright (c) 2014-2022,  Regents of the University of California,
+ * Copyright (c) 2014-2024,  Regents of the University of California,
  *                           Arizona Board of Regents,
  *                           Colorado State University,
  *                           University Pierre & Marie Curie, Sorbonne University,
@@ -48,6 +48,12 @@ class MulticastStrategy : public Strategy
   afterReceiveInterest(const Interest& interest, const FaceEndpoint& ingress,
                        const shared_ptr<pit::Entry>& pitEntry) override;
 
+  void
+  onInterestLoop(const Interest& interest, const FaceEndpoint& ingress) override
+  {
+    // do nothing
+  }
+
   void
   afterNewNextHop(const fib::NextHop& nextHop, const shared_ptr<pit::Entry>& pitEntry) override;
 

daemon/fw/strategy.cpp

@@ -1,6 +1,6 @@
 /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
 /*
- * Copyright (c) 2014-2022,  Regents of the University of California,
+ * Copyright (c) 2014-2024,  Regents of the University of California,
  *                           Arizona Board of Regents,
  *                           Colorado State University,
  *                           University Pierre & Marie Curie, Sorbonne University,
@@ -173,6 +173,16 @@ Strategy::Strategy(Forwarder& forwarder)
 
 Strategy::~Strategy() = default;
 
+void
+Strategy::onInterestLoop(const Interest& interest, const FaceEndpoint& ingress)
+{
+  NFD_LOG_DEBUG("onInterestLoop in=" << ingress << " name=" << interest.getName());
+
+  lp::Nack nack(interest);
+  nack.setReason(lp::NackReason::DUPLICATE);
+  this->sendNack(nack, ingress.face);
+}
+
 void
 Strategy::afterContentStoreHit(const Data& data, const FaceEndpoint& ingress,
                                const shared_ptr<pit::Entry>& pitEntry)

daemon/fw/strategy.hpp

@@ -135,7 +135,7 @@ class Strategy : noncopyable
    * The Interest:
    *  - has not exceeded HopLimit
    *  - does not violate Scope
-   *  - is not looped
+   *  - has not looped
    *  - cannot be satisfied by ContentStore
    *  - is under a namespace managed by this strategy
    *
@@ -160,6 +160,20 @@ class Strategy : noncopyable
   afterReceiveInterest(const Interest& interest, const FaceEndpoint& ingress,
                        const shared_ptr<pit::Entry>& pitEntry) = 0;
 
+  /**
+   * \brief Trigger after an Interest loop is detected.
+   *
+   * The Interest:
+   *  - has not exceeded HopLimit
+   *  - does not violate Scope
+   *  - has looped
+   *  - is under a namespace managed by this strategy
+   *
+   * In the base class, this method sends a Nack with reason DUPLICATE to \p ingress.
+   */
+  virtual void
+  onInterestLoop(const Interest& interest, const FaceEndpoint& ingress);
+
   /**
    * \brief Trigger after a matching Data is found in the Content Store.
    *
@@ -339,6 +353,21 @@ class Strategy : noncopyable
     return m_forwarder.onOutgoingNack(header, egress, pitEntry);
   }
 
+  /**
+   * \brief Send a Nack packet without going through the outgoing Nack pipeline.
+   *
+   * \param nack the Nack packet
+   * \param egress face through which to send out the Nack
+   * \return Whether the Nack was sent (true) or dropped (false)
+   */
+  NFD_VIRTUAL_WITH_TESTS bool
+  sendNack(const lp::Nack& nack, Face& egress)
+  {
+    egress.sendNack(nack);
+    ++m_forwarder.m_counters.nOutNacks;
+    return true;
+  }
+
   /**
    * \brief Send Nack to every face that has an in-record, except those in \p exceptFaces
    * \param header the Nack header

tests/daemon/fw/multicast-strategy.t.cpp

@@ -210,8 +210,28 @@ BOOST_AUTO_TEST_CASE(LoopingInterest)
   pitEntry->insertOrUpdateInRecord(*face1, *interest);
 
   strategy.afterReceiveInterest(*interest, FaceEndpoint(*face1), pitEntry);
-  BOOST_CHECK_EQUAL(strategy.rejectPendingInterestHistory.size(), 0);
-  BOOST_CHECK_EQUAL(strategy.sendInterestHistory.size(), 0);
+  BOOST_TEST(strategy.rejectPendingInterestHistory.size() == 0);
+  BOOST_TEST(strategy.sendInterestHistory.size() == 0);
+}
+
+BOOST_AUTO_TEST_CASE(DuplicateInterest)
+{
+  fib::Entry& fibEntry = *fib.insert(Name()).first;
+  fib.addOrUpdateNextHop(fibEntry, *face3, 0);
+
+  auto interest = makeInterest("ndn:/H0D6i5fc");
+
+  // first interest
+  forwarder.onIncomingInterest(*interest, FaceEndpoint(*face1));
+  BOOST_TEST(forwarder.getCounters().nInInterests == 1);
+  BOOST_TEST(strategy.sendInterestHistory.size() == 1);
+
+  // second interest (duplicate, should enter onInterestLoop)
+  forwarder.onIncomingInterest(*interest, FaceEndpoint(*face2));
+  BOOST_TEST(forwarder.getCounters().nInInterests == 2);
+  BOOST_TEST(strategy.sendInterestHistory.size() == 1);
+  BOOST_TEST(strategy.rejectPendingInterestHistory.size() == 0);
+  BOOST_TEST(strategy.sendNackHistory.size() == 0);
 }
 
 BOOST_AUTO_TEST_CASE(RetxSuppression)

tests/daemon/fw/strategy-instantiation.t.cpp

@@ -64,7 +64,7 @@ using Tests = boost::mp11::mp_list<
   Test<AccessStrategy, false, 1>,
   Test<AsfStrategy, true, 5>,
   Test<BestRouteStrategy, true, 5>,
-  Test<MulticastStrategy, true, 4>,
+  Test<MulticastStrategy, true, 5>,
   Test<SelfLearningStrategy, false, 1>,
   Test<RandomStrategy, false, 1>
 >;

tests/daemon/fw/strategy-tester.hpp

@@ -1,6 +1,6 @@
 /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
 /*
- * Copyright (c) 2014-2022,  Regents of the University of California,
+ * Copyright (c) 2014-2024,  Regents of the University of California,
  *                           Arizona Board of Regents,
  *                           Colorado State University,
  *                           University Pierre & Marie Curie, Sorbonne University,
@@ -99,8 +99,7 @@ class StrategyTester : public S
 
 protected:
   pit::OutRecord*
-  sendInterest(const Interest& interest, Face& egress,
-               const shared_ptr<pit::Entry>& pitEntry) override
+  sendInterest(const Interest& interest, Face& egress, const shared_ptr<pit::Entry>& pitEntry) override
   {
     sendInterestHistory.push_back({pitEntry->getInterest(), egress.getId(), interest});
     auto it = pitEntry->insertOrUpdateOutRecord(egress, interest);
@@ -117,15 +116,22 @@ class StrategyTester : public S
   }
 
   bool
-  sendNack(const lp::NackHeader& header, Face& egress,
-           const shared_ptr<pit::Entry>& pitEntry) override
+  sendNack(const lp::NackHeader& header, Face& egress, const shared_ptr<pit::Entry>& pitEntry) override
   {
     sendNackHistory.push_back({pitEntry->getInterest(), egress.getId(), header});
     pitEntry->deleteInRecord(egress);
     afterAction();
     return true;
   }
 
+  bool
+  sendNack(const lp::Nack& nack, Face& egress) override
+  {
+    sendNackHistory.push_back({nack.getInterest(), egress.getId(), nack.getHeader()});
+    afterAction();
+    return true;
+  }
+
 public:
   struct SendInterestArgs
   {