fw: always create PIT in-records with EndpointId=0 for now

Also improve test coverage

Refs: #4849
Change-Id: I0f3aba98ec7460f0dbf58b6dc00414e1f722c036

daemon/fw/forwarder.cpp

@@ -160,12 +160,6 @@ Forwarder::onInterestLoop(const FaceEndpoint& ingress, const Interest& interest)
   ingress.face.sendNack(nack);
 }
 
-static inline bool
-compare_InRecord_expiry(const pit::InRecord& a, const pit::InRecord& b)
-{
-  return a.getExpiry() < b.getExpiry();
-}
-
 void
 Forwarder::onContentStoreMiss(const FaceEndpoint& ingress,
                               const shared_ptr<pit::Entry>& pitEntry, const Interest& interest)
@@ -174,20 +168,27 @@ Forwarder::onContentStoreMiss(const FaceEndpoint& ingress,
   ++m_counters.nCsMisses;
 
   // insert in-record
-  pitEntry->insertOrUpdateInRecord(ingress.face, ingress.endpoint, interest);
+  // FIXME Strategies are not prepared to handle non-zero EndpointIds, so always insert
+  //       the in-record with EndpointId=0 for now. Eventually, this pipeline will need
+  //       to be refactored so that strategies can control the in-record insertion.
+  pitEntry->insertOrUpdateInRecord(ingress.face, 0, interest);
 
   // set PIT expiry timer to the time that the last PIT in-record expires
-  auto lastExpiring = std::max_element(pitEntry->in_begin(), pitEntry->in_end(), &compare_InRecord_expiry);
+  auto lastExpiring = std::max_element(pitEntry->in_begin(), pitEntry->in_end(),
+                                       [] (const auto& a, const auto& b) {
+                                         return a.getExpiry() < b.getExpiry();
+                                       });
   auto lastExpiryFromNow = lastExpiring->getExpiry() - time::steady_clock::now();
   this->setExpiryTimer(pitEntry, time::duration_cast<time::milliseconds>(lastExpiryFromNow));
 
   // has NextHopFaceId?
-  shared_ptr<lp::NextHopFaceIdTag> nextHopTag = interest.getTag<lp::NextHopFaceIdTag>();
+  auto nextHopTag = interest.getTag<lp::NextHopFaceIdTag>();
   if (nextHopTag != nullptr) {
     // chosen NextHop face exists?
     Face* nextHopFace = m_faceTable.get(*nextHopTag);
     if (nextHopFace != nullptr) {
-      NFD_LOG_DEBUG("onContentStoreMiss interest=" << interest.getName() << " nexthop-faceid=" << nextHopFace->getId());
+      NFD_LOG_DEBUG("onContentStoreMiss interest=" << interest.getName()
+                    << " nexthop-faceid=" << nextHopFace->getId());
       // go to outgoing Interest pipeline
       // scope control is unnecessary, because privileged app explicitly wants to forward
       this->onOutgoingInterest(pitEntry, FaceEndpoint(*nextHopFace, 0), interest);
@@ -208,7 +209,7 @@ Forwarder::onContentStoreHit(const FaceEndpoint& ingress, const shared_ptr<pit::
   ++m_counters.nCsHits;
 
   data.setTag(make_shared<lp::IncomingFaceIdTag>(face::FACEID_CONTENT_STORE));
-  // XXX should we lookup PIT for other Interests that also match csMatch?
+  // FIXME Should we lookup PIT for other Interests that also match the data?
 
   pitEntry->isSatisfied = true;
   pitEntry->dataFreshnessPeriod = data.getFreshnessPeriod();
@@ -314,7 +315,7 @@ Forwarder::onIncomingData(const FaceEndpoint& ingress, const Data& data)
     std::set<std::pair<Face*, EndpointId>> pendingDownstreams;
     auto now = time::steady_clock::now();
 
-    for (const shared_ptr<pit::Entry>& pitEntry : pitMatches) {
+    for (const auto& pitEntry : pitMatches) {
       NFD_LOG_DEBUG("onIncomingData matching=" << pitEntry->getName());
 
       // remember pending downstreams
@@ -403,8 +404,9 @@ Forwarder::onIncomingNack(const FaceEndpoint& ingress, const lp::Nack& nack)
 
   // if multi-access or ad hoc face, drop
   if (ingress.face.getLinkType() != ndn::nfd::LINK_TYPE_POINT_TO_POINT) {
-    NFD_LOG_DEBUG("onIncomingNack in=" << ingress << " nack=" << nack.getInterest().getName()
-                  << "~" << nack.getReason() << " face-is-multi-access");
+    NFD_LOG_DEBUG("onIncomingNack in=" << ingress
+                  << " nack=" << nack.getInterest().getName() << "~" << nack.getReason()
+                  << " link-type=" << ingress.face.getLinkType());
     return;
   }
 
@@ -418,7 +420,7 @@ Forwarder::onIncomingNack(const FaceEndpoint& ingress, const lp::Nack& nack)
   }
 
   // has out-record?
-  pit::OutRecordCollection::iterator outRecord = pitEntry->getOutRecord(ingress.face, ingress.endpoint);
+  auto outRecord = pitEntry->getOutRecord(ingress.face, ingress.endpoint);
   // if no out-record found, drop
   if (outRecord == pitEntry->out_end()) {
     NFD_LOG_DEBUG("onIncomingNack in=" << ingress << " nack=" << nack.getInterest().getName()
@@ -456,13 +458,12 @@ Forwarder::onOutgoingNack(const shared_ptr<pit::Entry>& pitEntry,
 {
   if (egress.face.getId() == face::INVALID_FACEID) {
     NFD_LOG_WARN("onOutgoingNack out=(invalid)"
-                 << " nack=" << pitEntry->getInterest().getName()
-                 << "~" << nack.getReason() << " no-in-record");
+                 << " nack=" << pitEntry->getInterest().getName() << "~" << nack.getReason());
     return;
   }
 
   // has in-record?
-  pit::InRecordCollection::iterator inRecord = pitEntry->getInRecord(egress.face, egress.endpoint);
+  auto inRecord = pitEntry->getInRecord(egress.face, egress.endpoint);
 
   // if no in-record found, drop
   if (inRecord == pitEntry->in_end()) {
@@ -475,8 +476,8 @@ Forwarder::onOutgoingNack(const shared_ptr<pit::Entry>& pitEntry,
   // if multi-access or ad hoc face, drop
   if (egress.face.getLinkType() != ndn::nfd::LINK_TYPE_POINT_TO_POINT) {
     NFD_LOG_DEBUG("onOutgoingNack out=" << egress
-                  << " nack=" << pitEntry->getInterest().getName()
-                  << "~" << nack.getReason() << " face-is-multi-access");
+                  << " nack=" << pitEntry->getInterest().getName() << "~" << nack.getReason()
+                  << " link-type=" << egress.face.getLinkType());
     return;
   }
 

tests/daemon/fw/broadcast-medium.t.cpp

@@ -0,0 +1,122 @@
+/* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
+/*
+ * Copyright (c) 2014-2019,  Regents of the University of California,
+ *                           Arizona Board of Regents,
+ *                           Colorado State University,
+ *                           University Pierre & Marie Curie, Sorbonne University,
+ *                           Washington University in St. Louis,
+ *                           Beijing Institute of Technology,
+ *                           The University of Memphis.
+ *
+ * This file is part of NFD (Named Data Networking Forwarding Daemon).
+ * See AUTHORS.md for complete list of NFD authors and contributors.
+ *
+ * NFD is free software: you can redistribute it and/or modify it under the terms
+ * of the GNU General Public License as published by the Free Software Foundation,
+ * either version 3 of the License, or (at your option) any later version.
+ *
+ * NFD is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY;
+ * without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR
+ * PURPOSE.  See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * NFD, e.g., in COPYING.md file.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+// Strategies that can correctly handle multi-access faces on a broadcast medium,
+// sorted alphabetically.
+#include "fw/asf-strategy.hpp"
+#include "fw/best-route-strategy2.hpp"
+#include "fw/multicast-strategy.hpp"
+
+#include "tests/daemon/global-io-fixture.hpp"
+#include "topology-tester.hpp"
+
+#include <boost/mpl/vector.hpp>
+
+namespace nfd {
+namespace fw {
+namespace tests {
+
+template<typename S>
+class BroadcastMediumFixture : public GlobalIoTimeFixture
+{
+protected:
+  BroadcastMediumFixture()
+  {
+    nodeC = topo.addForwarder("C");
+    nodeD = topo.addForwarder("D");
+    nodeP = topo.addForwarder("P");
+
+    for (TopologyNode node : {nodeC, nodeD, nodeP}) {
+      topo.setStrategy<S>(node);
+    }
+
+    auto w = topo.addLink("W", 10_ms, {nodeC, nodeD, nodeP}, ndn::nfd::LINK_TYPE_MULTI_ACCESS);
+    faceC = &w->getFace(nodeC);
+    faceD = &w->getFace(nodeD);
+    faceP = &w->getFace(nodeP);
+
+    appC = topo.addAppFace("consumer1", nodeC);
+    topo.registerPrefix(nodeC, *faceC, "/P");
+    topo.addIntervalConsumer(appC->getClientFace(), "/P", 0_ms, 1, 1);
+    appD = topo.addAppFace("consumer2", nodeD);
+    topo.registerPrefix(nodeD, *faceD, "/P");
+    topo.addIntervalConsumer(appD->getClientFace(), "/P", 0_ms, 1, 1);
+    appP = topo.addAppFace("producer", nodeP, "/P");
+    topo.addEchoProducer(appP->getClientFace(), "/P", 100_ms);
+  }
+
+protected:
+  TopologyTester topo;
+  TopologyNode nodeC;
+  TopologyNode nodeD;
+  TopologyNode nodeP;
+  Face* faceC;
+  Face* faceD;
+  Face* faceP;
+  shared_ptr<TopologyAppLink> appC;
+  shared_ptr<TopologyAppLink> appD;
+  shared_ptr<TopologyAppLink> appP;
+};
+
+BOOST_AUTO_TEST_SUITE(Fw)
+BOOST_AUTO_TEST_SUITE(TestBroadcastMedium)
+
+using Strategies = boost::mpl::vector<
+  AsfStrategy,
+  BestRouteStrategy2,
+  MulticastStrategy
+>;
+
+BOOST_FIXTURE_TEST_CASE_TEMPLATE(SameFaceDifferentEndpoint, S, Strategies, BroadcastMediumFixture<S>)
+{
+  //   C   D   P
+  //   |   |   |
+  // --+---+---+--
+  //
+  // C and D are consumers. P is the producer. They communicate with each other
+  // through a multi-access face over a (wired or wireless) broadcast medium.
+
+  this->advanceClocks(10_ms, 50_ms);
+
+  BOOST_CHECK_EQUAL(this->faceC->getCounters().nOutInterests, 1);
+  BOOST_CHECK_EQUAL(this->faceD->getCounters().nOutInterests, 1);
+  BOOST_CHECK_EQUAL(this->faceP->getCounters().nInInterests, 2);
+  BOOST_CHECK_EQUAL(this->faceC->getCounters().nInData, 0);
+  BOOST_CHECK_EQUAL(this->faceD->getCounters().nInData, 0);
+  BOOST_CHECK_EQUAL(this->faceP->getCounters().nOutData, 0);
+
+  this->advanceClocks(10_ms, 200_ms);
+
+  BOOST_CHECK_EQUAL(this->faceC->getCounters().nInData, 1);
+  BOOST_CHECK_EQUAL(this->faceD->getCounters().nInData, 1);
+  BOOST_CHECK_EQUAL(this->faceP->getCounters().nOutData, 1);
+}
+
+BOOST_AUTO_TEST_SUITE_END() // TestBroadcastMedium
+BOOST_AUTO_TEST_SUITE_END() // Fw
+
+} // namespace tests
+} // namespace fw
+} // namespace nfd

tests/daemon/fw/forwarder.t.cpp

@@ -180,26 +180,22 @@ BOOST_AUTO_TEST_CASE(NextHopFaceId)
 class ScopeLocalhostIncomingTestForwarder : public Forwarder
 {
 public:
-  ScopeLocalhostIncomingTestForwarder()
-  {
-  }
-
   void
-  onDataUnsolicited(const FaceEndpoint& ingress, const Data& data) override
+  onDataUnsolicited(const FaceEndpoint&, const Data&) final
   {
     ++onDataUnsolicited_count;
   }
 
 protected:
   void
-  dispatchToStrategy(pit::Entry&, std::function<void(fw::Strategy&)>) override
+  dispatchToStrategy(pit::Entry&, std::function<void(fw::Strategy&)>) final
   {
     ++dispatchToStrategy_count;
   }
 
 public:
-  int dispatchToStrategy_count;
-  int onDataUnsolicited_count;
+  int dispatchToStrategy_count = 0;
+  int onDataUnsolicited_count = 0;
 };
 
 BOOST_AUTO_TEST_CASE(ScopeLocalhostIncoming)

tests/daemon/table/pit-entry.t.cpp

@@ -66,20 +66,21 @@ BOOST_AUTO_TEST_CASE(CanMatch)
 
 BOOST_AUTO_TEST_CASE(InOutRecords)
 {
-  shared_ptr<Face> face1 = make_shared<DummyFace>();
-  shared_ptr<Face> face2 = make_shared<DummyFace>();
+  auto face1 = make_shared<DummyFace>();
+  auto face2 = make_shared<DummyFace>();
+
   Name name("ndn:/KuYfjtRq");
-  shared_ptr<Interest> interest  = makeInterest(name);
-  shared_ptr<Interest> interest1 = makeInterest(name);
+  auto interest = makeInterest(name);
+  auto interest1 = makeInterest(name);
   interest1->setInterestLifetime(2528_ms);
   interest1->setNonce(25559);
-  shared_ptr<Interest> interest2 = makeInterest(name);
+  auto interest2 = makeInterest(name);
   interest2->setInterestLifetime(6464_ms);
   interest2->setNonce(19004);
-  shared_ptr<Interest> interest3 = makeInterest(name);
+  auto interest3 = makeInterest(name);
   interest3->setInterestLifetime(3585_ms);
   interest3->setNonce(24216);
-  shared_ptr<Interest> interest4 = makeInterest(name);
+  auto interest4 = makeInterest(name);
   interest4->setInterestLifetime(8795_ms);
   interest4->setNonce(17365);
 
@@ -94,9 +95,9 @@ BOOST_AUTO_TEST_CASE(InOutRecords)
   BOOST_CHECK_EQUAL(outRecords1.size(), 0);
 
   // insert in-record
-  time::steady_clock::TimePoint before1 = time::steady_clock::now();
+  auto before1 = time::steady_clock::now();
   InRecordCollection::iterator in1 = entry.insertOrUpdateInRecord(*face1, 0, *interest1);
-  time::steady_clock::TimePoint after1 = time::steady_clock::now();
+  auto after1 = time::steady_clock::now();
   const InRecordCollection& inRecords2 = entry.getInRecords();
   BOOST_CHECK_EQUAL(inRecords2.size(), 1);
   BOOST_CHECK(in1 == inRecords2.begin());
@@ -104,15 +105,14 @@ BOOST_AUTO_TEST_CASE(InOutRecords)
   BOOST_CHECK_EQUAL(in1->getLastNonce(), interest1->getNonce());
   BOOST_CHECK_GE(in1->getLastRenewed(), before1);
   BOOST_CHECK_LE(in1->getLastRenewed(), after1);
-  BOOST_CHECK_LE(in1->getExpiry() - in1->getLastRenewed()
-                 - interest1->getInterestLifetime(),
-                 (after1 - before1));
+  BOOST_CHECK_LE(in1->getExpiry() - in1->getLastRenewed() - interest1->getInterestLifetime(),
+                 after1 - before1);
   BOOST_CHECK(in1 == entry.getInRecord(*face1, 0));
 
   // insert out-record
-  time::steady_clock::TimePoint before2 = time::steady_clock::now();
+  auto before2 = time::steady_clock::now();
   OutRecordCollection::iterator out1 = entry.insertOrUpdateOutRecord(*face1, 0, *interest1);
-  time::steady_clock::TimePoint after2 = time::steady_clock::now();
+  auto after2 = time::steady_clock::now();
   const OutRecordCollection& outRecords2 = entry.getOutRecords();
   BOOST_CHECK_EQUAL(outRecords2.size(), 1);
   BOOST_CHECK(out1 == outRecords2.begin());
@@ -120,44 +120,61 @@ BOOST_AUTO_TEST_CASE(InOutRecords)
   BOOST_CHECK_EQUAL(out1->getLastNonce(), interest1->getNonce());
   BOOST_CHECK_GE(out1->getLastRenewed(), before2);
   BOOST_CHECK_LE(out1->getLastRenewed(), after2);
-  BOOST_CHECK_LE(out1->getExpiry() - out1->getLastRenewed()
-                 - interest1->getInterestLifetime(),
-                 (after2 - before2));
+  BOOST_CHECK_LE(out1->getExpiry() - out1->getLastRenewed() - interest1->getInterestLifetime(),
+                 after2 - before2);
   BOOST_CHECK(out1 == entry.getOutRecord(*face1, 0));
 
   // update in-record
-  time::steady_clock::TimePoint before3 = time::steady_clock::now();
+  auto before3 = time::steady_clock::now();
   InRecordCollection::iterator in2 = entry.insertOrUpdateInRecord(*face1, 0, *interest2);
-  time::steady_clock::TimePoint after3 = time::steady_clock::now();
+  auto after3 = time::steady_clock::now();
   const InRecordCollection& inRecords3 = entry.getInRecords();
   BOOST_CHECK_EQUAL(inRecords3.size(), 1);
   BOOST_CHECK(in2 == inRecords3.begin());
   BOOST_CHECK_EQUAL(&in2->getFace(), face1.get());
   BOOST_CHECK_EQUAL(in2->getLastNonce(), interest2->getNonce());
-  BOOST_CHECK_LE(in2->getExpiry() - in2->getLastRenewed()
-                 - interest2->getInterestLifetime(),
-                 (after3 - before3));
+  BOOST_CHECK_LE(in2->getExpiry() - in2->getLastRenewed() - interest2->getInterestLifetime(),
+                 after3 - before3);
 
-  // insert another in-record
+  // insert another in-record (different face)
   InRecordCollection::iterator in3 = entry.insertOrUpdateInRecord(*face2, 0, *interest3);
   const InRecordCollection& inRecords4 = entry.getInRecords();
   BOOST_CHECK_EQUAL(inRecords4.size(), 2);
   BOOST_CHECK_EQUAL(&in3->getFace(), face2.get());
+  BOOST_CHECK_EQUAL(in3->getEndpointId(), 0);
+
+  // insert another in-record (different endpoint)
+  InRecordCollection::iterator in4 = entry.insertOrUpdateInRecord(*face1, 42, *interest4);
+  const InRecordCollection& inRecords5 = entry.getInRecords();
+  BOOST_CHECK_EQUAL(inRecords5.size(), 3);
+  BOOST_CHECK_EQUAL(&in4->getFace(), face1.get());
+  BOOST_CHECK_EQUAL(in4->getEndpointId(), 42);
 
   // get in-record
-  InRecordCollection::iterator in4 = entry.getInRecord(*face1, 0);
-  BOOST_REQUIRE(in4 != entry.in_end());
-  BOOST_CHECK_EQUAL(&in4->getFace(), face1.get());
+  InRecordCollection::iterator in5 = entry.getInRecord(*face1, 0);
+  BOOST_REQUIRE(in5 != entry.in_end());
+  BOOST_CHECK_EQUAL(&in5->getFace(), face1.get());
+  BOOST_CHECK_EQUAL(in5->getEndpointId(), 0);
+  InRecordCollection::iterator in6 = entry.getInRecord(*face1, 42);
+  BOOST_REQUIRE(in6 != entry.in_end());
+  BOOST_CHECK_EQUAL(&in6->getFace(), face1.get());
+  BOOST_CHECK_EQUAL(in6->getEndpointId(), 42);
+  BOOST_CHECK(in5 != in6);
+
+  // delete in-record
+  entry.deleteInRecord(*face1, 0);
+  BOOST_CHECK_EQUAL(entry.getInRecords().size(), 2);
+  BOOST_CHECK(entry.getInRecord(*face1, 0) == entry.in_end());
+  BOOST_CHECK(entry.getInRecord(*face1, 42) != entry.in_end());
 
   // clear in-records
   entry.clearInRecords();
-  const InRecordCollection& inRecords5 = entry.getInRecords();
-  BOOST_CHECK_EQUAL(inRecords5.size(), 0);
+  BOOST_CHECK_EQUAL(entry.getInRecords().size(), 0);
   BOOST_CHECK(entry.getInRecord(*face1, 0) == entry.in_end());
+  BOOST_CHECK(entry.getInRecord(*face1, 42) == entry.in_end());
 
   // insert another out-record
-  OutRecordCollection::iterator out2 =
-    entry.insertOrUpdateOutRecord(*face2, 0, *interest4);
+  OutRecordCollection::iterator out2 = entry.insertOrUpdateOutRecord(*face2, 0, *interest4);
   const OutRecordCollection& outRecords3 = entry.getOutRecords();
   BOOST_CHECK_EQUAL(outRecords3.size(), 2);
   BOOST_CHECK_EQUAL(&out2->getFace(), face2.get());
@@ -177,30 +194,29 @@ BOOST_AUTO_TEST_CASE(InOutRecords)
 
 BOOST_AUTO_TEST_CASE(Lifetime)
 {
-  shared_ptr<Interest> interest = makeInterest("ndn:/7oIEurbgy6");
-
-  shared_ptr<Face> face = make_shared<DummyFace>();
+  auto interest = makeInterest("ndn:/7oIEurbgy6");
+  auto face = make_shared<DummyFace>();
   Entry entry(*interest);
 
-  InRecordCollection::iterator inIt = entry.insertOrUpdateInRecord(*face, 0, *interest);
+  auto inIt = entry.insertOrUpdateInRecord(*face, 0, *interest);
   BOOST_CHECK_GT(inIt->getExpiry(), time::steady_clock::now());
 
-  OutRecordCollection::iterator outIt = entry.insertOrUpdateOutRecord(*face, 0, *interest);
+  auto outIt = entry.insertOrUpdateOutRecord(*face, 0, *interest);
   BOOST_CHECK_GT(outIt->getExpiry(), time::steady_clock::now());
 }
 
 BOOST_AUTO_TEST_CASE(OutRecordNack)
 {
-  shared_ptr<Face> face1 = make_shared<DummyFace>();
+  auto face1 = make_shared<DummyFace>();
   OutRecord outR(*face1, 0);
   BOOST_CHECK(outR.getIncomingNack() == nullptr);
 
-  shared_ptr<Interest> interest1 = makeInterest("ndn:/uWiapGjYL");
+  auto interest1 = makeInterest("ndn:/uWiapGjYL");
   interest1->setNonce(165);
   outR.update(*interest1);
   BOOST_CHECK(outR.getIncomingNack() == nullptr);
 
-  shared_ptr<Interest> interest2 = makeInterest("ndn:/uWiapGjYL");
+  auto interest2 = makeInterest("ndn:/uWiapGjYL");
   interest2->setNonce(996);
   lp::Nack nack2(*interest2);
   nack2.setReason(lp::NackReason::CONGESTION);