fw: FaceTable iterator dereferences to Face& instead of shared_ptr

This commit also improves the speed and reliability of
Mgmt/TestFaceManager/CreateFace test suite.

refs #3205

Change-Id: Idd013488ced2d7f8072ef8a3d910f94da2e0c8ac

daemon/fw/face-table.cpp

@@ -120,7 +120,7 @@ FaceTable::remove(shared_ptr<Face> face)
 FaceTable::ForwardRange
 FaceTable::getForwardRange() const
 {
-  return m_faces | boost::adaptors::map_values;
+  return m_faces | boost::adaptors::map_values | boost::adaptors::indirected;
 }
 
 FaceTable::const_iterator

daemon/fw/face-table.hpp

@@ -27,6 +27,7 @@
 #define NFD_DAEMON_FW_FACE_TABLE_HPP
 
 #include "face/face.hpp"
+#include <boost/range/adaptor/indirected.hpp>
 #include <boost/range/adaptor/map.hpp>
 
 namespace nfd {
@@ -72,9 +73,9 @@ public:
 public: // enumeration
   typedef std::map<FaceId, shared_ptr<Face>> FaceMap;
 
-  typedef boost::select_second_const_range<FaceMap> ForwardRange;
+  typedef boost::indirected_range<const boost::select_second_const_range<FaceMap>> ForwardRange;
 
-  /** \brief ForwardIterator for shared_ptr<Face>
+  /** \brief ForwardIterator for Face&
    */
   typedef boost::range_iterator<ForwardRange>::type const_iterator;
 

daemon/mgmt/face-manager.cpp

@@ -249,8 +249,8 @@ FaceManager::listFaces(const Name& topPrefix, const Interest& interest,
                        ndn::mgmt::StatusDatasetContext& context)
 {
   auto now = time::steady_clock::now();
-  for (const auto& face : m_faceTable) {
-    ndn::nfd::FaceStatus status = collectFaceStatus(*face, now);
+  for (const Face& face : m_faceTable) {
+    ndn::nfd::FaceStatus status = collectFaceStatus(face, now);
     context.append(status.wireEncode());
   }
   context.end();
@@ -294,11 +294,11 @@ FaceManager::queryFaces(const Name& topPrefix, const Interest& interest,
   }
 
   auto now = time::steady_clock::now();
-  for (const auto& face : m_faceTable) {
-    if (!doesMatchFilter(faceFilter, face)) {
+  for (const Face& face : m_faceTable) {
+    if (!matchFilter(faceFilter, face)) {
       continue;
     }
-    ndn::nfd::FaceStatus status = collectFaceStatus(*face, now);
+    ndn::nfd::FaceStatus status = collectFaceStatus(face, now);
     context.append(status.wireEncode());
   }
 
@@ -306,41 +306,41 @@ FaceManager::queryFaces(const Name& topPrefix, const Interest& interest,
 }
 
 bool
-FaceManager::doesMatchFilter(const ndn::nfd::FaceQueryFilter& filter, shared_ptr<Face> face)
+FaceManager::matchFilter(const ndn::nfd::FaceQueryFilter& filter, const Face& face)
 {
   if (filter.hasFaceId() &&
-      filter.getFaceId() != static_cast<uint64_t>(face->getId())) {
+      filter.getFaceId() != static_cast<uint64_t>(face.getId())) {
     return false;
   }
 
   if (filter.hasUriScheme() &&
-      filter.getUriScheme() != face->getRemoteUri().getScheme() &&
-      filter.getUriScheme() != face->getLocalUri().getScheme()) {
+      filter.getUriScheme() != face.getRemoteUri().getScheme() &&
+      filter.getUriScheme() != face.getLocalUri().getScheme()) {
     return false;
   }
 
   if (filter.hasRemoteUri() &&
-      filter.getRemoteUri() != face->getRemoteUri().toString()) {
+      filter.getRemoteUri() != face.getRemoteUri().toString()) {
     return false;
   }
 
   if (filter.hasLocalUri() &&
-      filter.getLocalUri() != face->getLocalUri().toString()) {
+      filter.getLocalUri() != face.getLocalUri().toString()) {
     return false;
   }
 
   if (filter.hasFaceScope() &&
-      filter.getFaceScope() != face->getScope()) {
+      filter.getFaceScope() != face.getScope()) {
     return false;
   }
 
   if (filter.hasFacePersistency() &&
-      filter.getFacePersistency() != face->getPersistency()) {
+      filter.getFacePersistency() != face.getPersistency()) {
     return false;
   }
 
   if (filter.hasLinkType() &&
-      filter.getLinkType() != face->getLinkType()) {
+      filter.getLinkType() != face.getLinkType()) {
     return false;
   }
 

daemon/mgmt/face-manager.hpp

@@ -105,7 +105,7 @@ PUBLIC_WITH_TESTS_ELSE_PRIVATE: // StatusDataset
 
 private: // helpers for StatusDataset handler
   bool
-  doesMatchFilter(const ndn::nfd::FaceQueryFilter& filter, shared_ptr<Face> face);
+  matchFilter(const ndn::nfd::FaceQueryFilter& filter, const Face& face);
 
   /** \brief get status of face, including properties and counters
    */

tests/daemon/fw/face-table.t.cpp

@@ -113,41 +113,31 @@ BOOST_AUTO_TEST_CASE(Enumerate)
   BOOST_CHECK_EQUAL(std::distance(faceTable.begin(), faceTable.end()), faceTable.size());
   hasFace1 = hasFace2 = false;
   for (FaceTable::const_iterator it = faceTable.begin(); it != faceTable.end(); ++it) {
-    if (*it == face1) {
-      hasFace1 = true;
-    }
+    hasFace1 = hasFace1 || &*it == face1.get();
   }
-  BOOST_CHECK(hasFace1);
+  BOOST_CHECK_EQUAL(hasFace1, true);
 
   faceTable.add(face2);
   BOOST_CHECK_EQUAL(faceTable.size(), 2);
   BOOST_CHECK_EQUAL(std::distance(faceTable.begin(), faceTable.end()), faceTable.size());
   hasFace1 = hasFace2 = false;
   for (FaceTable::const_iterator it = faceTable.begin(); it != faceTable.end(); ++it) {
-    if (*it == face1) {
-      hasFace1 = true;
-    }
-    if (*it == face2) {
-      hasFace2 = true;
-    }
+    hasFace1 = hasFace1 || &*it == face1.get();
+    hasFace2 = hasFace2 || &*it == face2.get();
   }
-  BOOST_CHECK(hasFace1);
-  BOOST_CHECK(hasFace2);
+  BOOST_CHECK_EQUAL(hasFace1, true);
+  BOOST_CHECK_EQUAL(hasFace2, true);
 
   face1->close();
   BOOST_CHECK_EQUAL(faceTable.size(), 1);
   BOOST_CHECK_EQUAL(std::distance(faceTable.begin(), faceTable.end()), faceTable.size());
   hasFace1 = hasFace2 = false;
   for (FaceTable::const_iterator it = faceTable.begin(); it != faceTable.end(); ++it) {
-    if (*it == face1) {
-      hasFace1 = true;
-    }
-    if (*it == face2) {
-      hasFace2 = true;
-    }
+    hasFace1 = hasFace1 || &*it == face1.get();
+    hasFace2 = hasFace2 || &*it == face2.get();
   }
-  BOOST_CHECK(!hasFace1);
-  BOOST_CHECK(hasFace2);
+  BOOST_CHECK_EQUAL(hasFace1, false);
+  BOOST_CHECK_EQUAL(hasFace2, true);
 }
 
 BOOST_AUTO_TEST_SUITE_END() // TestFaceTable

tests/daemon/fw/strategy.t.cpp

@@ -1,6 +1,6 @@
 /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
 /**
- * Copyright (c) 2014-2015,  Regents of the University of California,
+ * Copyright (c) 2014-2016,  Regents of the University of California,
  *                           Arizona Board of Regents,
  *                           Colorado State University,
  *                           University Pierre & Marie Curie, Sorbonne University,
@@ -27,6 +27,8 @@
 #include "dummy-strategy.hpp"
 #include "tests/daemon/face/dummy-face.hpp"
 #include <boost/range/adaptor/filtered.hpp>
+#include <boost/range/adaptor/transformed.hpp>
+#include <boost/range/algorithm/copy.hpp>
 
 #include "tests/test-common.hpp"
 
@@ -58,14 +60,15 @@ public:
   getLocalFaces()
   {
     auto enumerable = this->getFaceTable() |
-                      boost::adaptors::filtered([] (shared_ptr<Face> face) {
-                        return face->getScope() == ndn::nfd::FACE_SCOPE_LOCAL;
+                      boost::adaptors::filtered([] (Face& face) {
+                        return face.getScope() == ndn::nfd::FACE_SCOPE_LOCAL;
+                      }) |
+                      boost::adaptors::transformed([] (Face& face) {
+                        return face.getId();
                       });
 
     std::vector<FaceId> results;
-    for (shared_ptr<Face> face : enumerable) {
-      results.push_back(face->getId());
-    }
+    boost::copy(enumerable, std::back_inserter(results));
     return results;
   }
 

tests/daemon/mgmt/face-manager-create-face.t.cpp

@@ -25,19 +25,18 @@
 
 #include "mgmt/face-manager.hpp"
 #include "fw/forwarder.hpp"
+#include <ndn-cxx/mgmt/dispatcher.hpp>
+#include <ndn-cxx/util/dummy-client-face.hpp>
+
+#include <thread>
+#include <boost/property_tree/info_parser.hpp>
 
 #include "tests/test-common.hpp"
 #include "tests/identity-management-fixture.hpp"
 
-#include <ndn-cxx/mgmt/dispatcher.hpp>
-#include <ndn-cxx/util/dummy-client-face.hpp>
-
-#include <boost/property_tree/info_parser.hpp>
-
 namespace nfd {
 namespace tests {
 
-
 BOOST_AUTO_TEST_SUITE(Mgmt)
 BOOST_AUTO_TEST_SUITE(TestFaceManager)
 
@@ -47,9 +46,10 @@ class FaceManagerNode
 {
 public:
   FaceManagerNode(ndn::KeyChain& keyChain, const std::string& port = "6363")
-    : face(getGlobalIoService(), keyChain, {true, true})
+    : faceTable(forwarder.getFaceTable())
+    , face(getGlobalIoService(), keyChain, {true, true})
     , dispatcher(face, keyChain, ndn::security::SigningInfo())
-    , manager(forwarder.getFaceTable(), dispatcher, validator)
+    , manager(faceTable, dispatcher, validator)
   {
     dispatcher.addTopPrefix("/localhost/nfd");
 
@@ -95,16 +95,17 @@ public:
   void
   closeFaces()
   {
-    std::vector<shared_ptr<Face>> facesToClose;
+    std::vector<std::reference_wrapper<Face>> facesToClose;
     std::copy(forwarder.getFaceTable().begin(), forwarder.getFaceTable().end(),
               std::back_inserter(facesToClose));
-    for (auto face : facesToClose) {
-      face->close();
+    for (Face& face : facesToClose) {
+      face.close();
     }
   }
 
 public:
   Forwarder forwarder;
+  FaceTable& faceTable;
   ndn::util::DummyClientFace face;
   ndn::mgmt::Dispatcher dispatcher;
   CommandValidator validator;
@@ -119,14 +120,16 @@ public:
     : node1(m_keyChain, "16363")
     , node2(m_keyChain, "26363")
   {
-    advanceClocks(time::milliseconds(1), 100);
+    advanceClocks(time::milliseconds(1), 5);
   }
 
   ~FaceManagerFixture()
   {
+    // Explicitly closing faces is necessary. Otherwise, in a subsequent test case,
+    // incoming packets may be delivered to an old socket from previous test cases.
     node1.closeFaces();
     node2.closeFaces();
-    advanceClocks(time::milliseconds(1), 100);
+    advanceClocks(time::milliseconds(1), 5);
   }
 
 public:
@@ -262,13 +265,11 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE(NewFace, T, Faces, FaceManagerFixture)
   Name commandName("/localhost/nfd/faces");
   commandName.append("create");
   commandName.append(FaceType().getParameters().wireEncode());
-
-  shared_ptr<Interest> command(make_shared<Interest>(commandName));
+  auto command = makeInterest(commandName);
   m_keyChain.sign(*command);
 
   bool hasCallbackFired = false;
   this->node1.face.onSendData.connect([this, command, &hasCallbackFired] (const Data& response) {
-      // std::cout << response << std::endl;
       if (!command->getName().isPrefixOf(response.getName())) {
         return;
       }
@@ -289,7 +290,7 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE(NewFace, T, Faces, FaceManagerFixture)
     });
 
   this->node1.face.receive(*command);
-  this->advanceClocks(time::milliseconds(1), 100);
+  this->advanceClocks(time::milliseconds(1), 5);
 
   BOOST_CHECK(hasCallbackFired);
 }
@@ -313,12 +314,11 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE(ExistingFace, T, FaceTransitions, FaceManagerFi
     Name commandName("/localhost/nfd/faces");
     commandName.append("create");
     commandName.append(FaceType1().getParameters().wireEncode());
-
-    shared_ptr<Interest> command(make_shared<Interest>(commandName));
+    auto command = makeInterest(commandName);
     m_keyChain.sign(*command);
 
     this->node1.face.receive(*command);
-    this->advanceClocks(time::milliseconds(1), 10);
+    this->advanceClocks(time::milliseconds(1), 5);
   }
 
   //
@@ -328,8 +328,7 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE(ExistingFace, T, FaceTransitions, FaceManagerFi
     Name commandName("/localhost/nfd/faces");
     commandName.append("create");
     commandName.append(FaceType2().getParameters().wireEncode());
-
-    shared_ptr<Interest> command(make_shared<Interest>(commandName));
+    auto command = makeInterest(commandName);
     m_keyChain.sign(*command);
 
     bool hasCallbackFired = false;
@@ -349,7 +348,7 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE(ExistingFace, T, FaceTransitions, FaceManagerFi
       });
 
     this->node1.face.receive(*command);
-    this->advanceClocks(time::milliseconds(1), 10);
+    this->advanceClocks(time::milliseconds(1), 5);
 
     BOOST_CHECK(hasCallbackFired);
   }
@@ -389,8 +388,7 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE(ExistingFaceOnDemand, T, OnDemandFaceTransition
     Name commandName("/localhost/nfd/faces");
     commandName.append("create");
     commandName.append(OtherNodeFace().getParameters().wireEncode());
-
-    shared_ptr<Interest> command(make_shared<Interest>(commandName));
+    auto command = makeInterest(commandName);
     m_keyChain.sign(*command);
 
     ndn::util::signal::ScopedConnection connection =
@@ -411,19 +409,21 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE(ExistingFaceOnDemand, T, OnDemandFaceTransition
         });
 
     this->node2.face.receive(*command);
-    this->advanceClocks(time::milliseconds(1), 10);
+    this->advanceClocks(time::milliseconds(1), 5); // let node2 process command and send Interest
+    std::this_thread::sleep_for(std::chrono::milliseconds(100)); // allow wallclock time for socket IO
+    this->advanceClocks(time::milliseconds(1), 5); // let node1 accept Interest and create on-demand face
   }
 
   // make sure there is on-demand face
-  bool onDemandFaceFound = false;
   FaceUri onDemandFaceUri(FaceType().getParameters().getUri());
-  for (auto face : this->node1.forwarder.getFaceTable()) {
-    if (face->getRemoteUri() == onDemandFaceUri) {
-      onDemandFaceFound = true;
+  const Face* foundFace = nullptr;
+  for (const Face& face : this->node1.faceTable) {
+    if (face.getRemoteUri() == onDemandFaceUri) {
+      foundFace = &face;
       break;
     }
   }
-  BOOST_REQUIRE(onDemandFaceFound);
+  BOOST_REQUIRE_MESSAGE(foundFace != nullptr, "on-demand face is not created");
 
   //
   {
@@ -432,12 +432,12 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE(ExistingFaceOnDemand, T, OnDemandFaceTransition
     Name commandName("/localhost/nfd/faces");
     commandName.append("create");
     commandName.append(FaceType().getParameters().wireEncode());
-
-    shared_ptr<Interest> command(make_shared<Interest>(commandName));
+    auto command = makeInterest(commandName);
     m_keyChain.sign(*command);
 
     bool hasCallbackFired = false;
-    this->node1.face.onSendData.connect([this, command, &hasCallbackFired] (const Data& response) {
+    this->node1.face.onSendData.connect(
+      [this, command, &hasCallbackFired, foundFace] (const Data& response) {
         if (!command->getName().isPrefixOf(response.getName())) {
           return;
         }
@@ -447,13 +447,15 @@ BOOST_FIXTURE_TEST_CASE_TEMPLATE(ExistingFaceOnDemand, T, OnDemandFaceTransition
 
         ControlParameters expectedParams(FaceType().getParameters());
         ControlParameters actualParams(actual.getBody());
-        BOOST_CHECK_EQUAL(expectedParams.getFacePersistency(), actualParams.getFacePersistency());
+        BOOST_CHECK_EQUAL(actualParams.getFacePersistency(), expectedParams.getFacePersistency());
+        BOOST_CHECK_EQUAL(actualParams.getFaceId(), foundFace->getId());
+        BOOST_CHECK_EQUAL(foundFace->getPersistency(), expectedParams.getFacePersistency());
 
         hasCallbackFired = true;
       });
 
     this->node1.face.receive(*command);
-    this->advanceClocks(time::milliseconds(1), 10);
+    this->advanceClocks(time::milliseconds(1), 5);
 
     BOOST_CHECK(hasCallbackFired);
   }