rib: change FIB mocking mechanism

Previously, RIB test suites mock FIB updates by invoking RibManager
or Rib's private methods from a callback. In this commit, Rib class
exposes a `mockFibResponse` function variable, so that test cases
can directly specify FibUpdater's response without needing to invoke
private methods. This further allows replacing these private methods
with lambdas.

Also, logic for beginning RibUpdateBatch is abstracted out of control
command handlers, in preparation for PrefixAnnouncement-based RIB
update functions.

refs #4683

Change-Id: I0c95d8daa1458c704c7352a7809e6bed69134642

rib/rib-manager.cpp

@@ -103,40 +103,112 @@ void
 RibManager::enableLocalFields()
 {
   m_nfdController.start<ndn::nfd::FaceUpdateCommand>(
-    ControlParameters()
-      .setFlagBit(ndn::nfd::BIT_LOCAL_FIELDS_ENABLED, true),
-    bind(&RibManager::onEnableLocalFieldsSuccess, this),
-    bind(&RibManager::onEnableLocalFieldsError, this, _1));
+    ControlParameters().setFlagBit(ndn::nfd::BIT_LOCAL_FIELDS_ENABLED, true),
+    [] (const ControlParameters& res) {
+      NFD_LOG_DEBUG("Local fields enabled");
+    },
+    [] (const ControlResponse& res) {
+      BOOST_THROW_EXCEPTION(Error("Couldn't enable local fields (" + to_string(res.getCode()) +
+                                  " " + res.getText() + ")"));
+    });
 }
 
 void
-RibManager::onRibUpdateSuccess(const RibUpdate& update)
+RibManager::beginAddRoute(const Name& name, Route route, optional<time::nanoseconds> expires,
+                          const std::function<void(RibUpdateResult)>& done)
 {
-  NFD_LOG_DEBUG("RIB update succeeded for " << update);
+  if (expires) {
+    if (*expires <= 0_ns) {
+      done(RibUpdateResult::EXPIRED);
+      return;
+    }
+    route.expires = time::steady_clock::now() + *expires;
+  }
+  else if (route.expires) {
+    expires = *route.expires - time::steady_clock::now();
+    if (*expires <= 0_ns) {
+      done(RibUpdateResult::EXPIRED);
+      return;
+    }
+  }
+
+  NFD_LOG_INFO("Adding route " << name << " nexthop=" << route.faceId <<
+               " origin=" << route.origin << " cost=" << route.cost);
+
+  if (expires) {
+    route.setExpirationEvent(scheduler::schedule(
+      *expires, [=] { m_rib.onRouteExpiration(name, route); }));
+    NFD_LOG_TRACE("Scheduled unregistration at: " << *route.expires);
+  }
+
+  m_registeredFaces.insert(route.faceId);
+
+  RibUpdate update;
+  update.setAction(RibUpdate::REGISTER)
+        .setName(name)
+        .setRoute(route);
+  beginRibUpdate(update, done);
 }
 
 void
-RibManager::onRibUpdateFailure(const RibUpdate& update, uint32_t code, const std::string& error)
+RibManager::beginRemoveRoute(const Name& name, const Route& route,
+                             const std::function<void(RibUpdateResult)>& done)
 {
-  NFD_LOG_DEBUG("RIB update failed for " << update << " (code: " << code
-                                                   << ", error: " << error << ")");
+  NFD_LOG_INFO("Removing route " << name << " nexthop=" << route.faceId <<
+               " origin=" << route.origin);
 
-  // Since the FIB rejected the update, clean up invalid routes
-  scheduleActiveFaceFetch(time::seconds(1));
+  RibUpdate update;
+  update.setAction(RibUpdate::UNREGISTER)
+        .setName(name)
+        .setRoute(route);
+  beginRibUpdate(update, done);
+}
+
+void
+RibManager::beginRibUpdate(const RibUpdate& update,
+                           const std::function<void(RibUpdateResult)>& done)
+{
+  m_rib.beginApplyUpdate(update,
+    [=] {
+      NFD_LOG_DEBUG("RIB update succeeded for " << update);
+      done(RibUpdateResult::OK);
+    },
+    [=] (uint32_t code, const std::string& error) {
+      NFD_LOG_DEBUG("RIB update failed for " << update << " (" << code << " " << error << ")");
+
+      // Since the FIB rejected the update, clean up invalid routes
+      scheduleActiveFaceFetch(1_s);
+
+      done(RibUpdateResult::ERROR);
+    });
 }
 
 void
 RibManager::registerTopPrefix(const Name& topPrefix)
 {
-  // register entry to the FIB
+  // add FIB nexthop
   m_nfdController.start<ndn::nfd::FibAddNextHopCommand>(
-    ControlParameters()
-        .setName(Name(topPrefix).append(MGMT_MODULE_NAME))
-        .setFaceId(0),
-    [=] (const auto& res) { this->onCommandPrefixAddNextHopSuccess(topPrefix, res); },
-    [=] (const auto& res) { this->onCommandPrefixAddNextHopError(topPrefix, res); });
+    ControlParameters().setName(Name(topPrefix).append(MGMT_MODULE_NAME))
+                       .setFaceId(0),
+    [=] (const ControlParameters& res) {
+      NFD_LOG_DEBUG("Successfully registered " << topPrefix << " with NFD");
 
-  // add top prefix to the dispatcher
+      // Routes must be inserted into the RIB so route flags can be applied
+      Route route;
+      route.faceId = res.getFaceId();
+      route.origin = ndn::nfd::ROUTE_ORIGIN_APP;
+      route.flags = ndn::nfd::ROUTE_FLAG_CHILD_INHERIT;
+
+      m_rib.insert(topPrefix, route);
+
+      m_registeredFaces.insert(route.faceId);
+    },
+    [=] (const ControlResponse& res) {
+      BOOST_THROW_EXCEPTION(Error("Cannot add FIB entry " + topPrefix.toUri() + " (" +
+                                  to_string(res.getCode()) + " " + res.getText() + ")"));
+    });
+
+  // add top prefix to the dispatcher without prefix registration
   m_dispatcher.addTopPrefix(topPrefix, false);
 }
 
@@ -162,38 +234,13 @@ RibManager::registerEntry(const Name& topPrefix, const Interest& interest,
   route.cost = parameters.getCost();
   route.flags = parameters.getFlags();
 
+  optional<time::nanoseconds> expires;
   if (parameters.hasExpirationPeriod() &&
       parameters.getExpirationPeriod() != time::milliseconds::max()) {
-    route.expires = time::steady_clock::now() + parameters.getExpirationPeriod();
-
-    // Schedule a new event, the old one will be cancelled during rib insertion.
-    scheduler::EventId eventId = scheduler::schedule(parameters.getExpirationPeriod(),
-      bind(&Rib::onRouteExpiration, &m_rib, parameters.getName(), route));
-
-    NFD_LOG_TRACE("Scheduled unregistration at: " << *route.expires <<
-                  " with EventId: " << eventId);
-
-    // Set the  NewEventId of this entry
-    route.setExpirationEvent(eventId);
-  }
-  else {
-    route.expires = nullopt;
+    expires = time::duration_cast<time::nanoseconds>(parameters.getExpirationPeriod());
   }
 
-  NFD_LOG_INFO("Adding route " << parameters.getName() << " nexthop=" << route.faceId
-                                                       << " origin=" << route.origin
-                                                       << " cost=" << route.cost);
-
-  RibUpdate update;
-  update.setAction(RibUpdate::REGISTER)
-        .setName(parameters.getName())
-        .setRoute(route);
-
-  m_rib.beginApplyUpdate(update,
-                         bind(&RibManager::onRibUpdateSuccess, this, update),
-                         bind(&RibManager::onRibUpdateFailure, this, update, _1, _2));
-
-  m_registeredFaces.insert(route.faceId);
+  beginAddRoute(parameters.getName(), std::move(route), expires, [] (RibUpdateResult) {});
 }
 
 void
@@ -210,17 +257,7 @@ RibManager::unregisterEntry(const Name& topPrefix, const Interest& interest,
   route.faceId = parameters.getFaceId();
   route.origin = parameters.getOrigin();
 
-  NFD_LOG_INFO("Removing route " << parameters.getName() << " nexthop=" << route.faceId
-                                                         << " origin=" << route.origin);
-
-  RibUpdate update;
-  update.setAction(RibUpdate::UNREGISTER)
-        .setName(parameters.getName())
-        .setRoute(route);
-
-  m_rib.beginApplyUpdate(update,
-                         bind(&RibManager::onRibUpdateSuccess, this, update),
-                         bind(&RibManager::onRibUpdateFailure, this, update, _1, _2));
+  beginRemoveRoute(parameters.getName(), route, [] (RibUpdateResult) {});
 }
 
 void
@@ -348,44 +385,5 @@ RibManager::onNotification(const ndn::nfd::FaceEventNotification& notification)
   }
 }
 
-void
-RibManager::onCommandPrefixAddNextHopSuccess(const Name& prefix,
-                                             const ndn::nfd::ControlParameters& result)
-{
-  NFD_LOG_DEBUG("Successfully registered " + prefix.toUri() + " with NFD");
-
-  // Routes must be inserted into the RIB so route flags can be applied
-  Route route;
-  route.faceId = result.getFaceId();
-  route.origin = ndn::nfd::ROUTE_ORIGIN_APP;
-  route.expires = nullopt;
-  route.flags = ndn::nfd::ROUTE_FLAG_CHILD_INHERIT;
-
-  m_rib.insert(prefix, route);
-
-  m_registeredFaces.insert(route.faceId);
-}
-
-void
-RibManager::onCommandPrefixAddNextHopError(const Name& name,
-                                           const ndn::nfd::ControlResponse& response)
-{
-  BOOST_THROW_EXCEPTION(Error("Error in setting interest filter (" + name.toUri() +
-                              "): " + response.getText()));
-}
-
-void
-RibManager::onEnableLocalFieldsSuccess()
-{
-  NFD_LOG_DEBUG("Local fields enabled");
-}
-
-void
-RibManager::onEnableLocalFieldsError(const ndn::nfd::ControlResponse& response)
-{
-  BOOST_THROW_EXCEPTION(Error("Couldn't enable local fields (code: " +
-                              to_string(response.getCode()) + ", info: " + response.getText() + ")"));
-}
-
 } // namespace rib
 } // namespace nfd

rib/rib-manager.hpp

@@ -87,28 +87,56 @@ public:
   void
   enableLocalFields();
 
-PUBLIC_WITH_TESTS_ELSE_PRIVATE:
+private: // RIB and FibUpdater actions
+  enum class RibUpdateResult
+  {
+    OK,
+    ERROR,
+    EXPIRED,
+  };
+
+  /** \brief Start adding a route to RIB and FIB.
+   *  \param name route name
+   *  \param route route parameters; may contain absolute expiration time
+   *  \param expires relative expiration time; if specified, overwrites \c route.expires
+   *  \param done completion callback
+   */
   void
-  onRibUpdateSuccess(const RibUpdate& update);
+  beginAddRoute(const Name& name, Route route, optional<time::nanoseconds> expires,
+                const std::function<void(RibUpdateResult)>& done);
+
+  /** \brief Start removing a route from RIB and FIB.
+   *  \param name route name
+   *  \param route route parameters
+   *  \param done completion callback
+   */
+  void
+  beginRemoveRoute(const Name& name, const Route& route,
+                   const std::function<void(RibUpdateResult)>& done);
 
   void
-  onRibUpdateFailure(const RibUpdate& update, uint32_t code, const std::string& error);
+  beginRibUpdate(const RibUpdate& update, const std::function<void(RibUpdateResult)>& done);
 
-private: // initialization helpers
+private: // management Dispatcher related
   void
   registerTopPrefix(const Name& topPrefix);
 
-private: // ControlCommand and StatusDataset
+  /** \brief Serve rib/register command.
+   */
   void
   registerEntry(const Name& topPrefix, const Interest& interest,
                 ControlParameters parameters,
                 const ndn::mgmt::CommandContinuation& done);
 
+  /** \brief Serve rib/unregister command.
+   */
   void
   unregisterEntry(const Name& topPrefix, const Interest& interest,
                   ControlParameters parameters,
                   const ndn::mgmt::CommandContinuation& done);
 
+  /** \brief Serve rib/list dataset.
+   */
   void
   listEntries(const Name& topPrefix, const Interest& interest,
               ndn::mgmt::StatusDatasetContext& context);
@@ -149,19 +177,6 @@ PUBLIC_WITH_TESTS_ELSE_PRIVATE:
   void
   onNotification(const ndn::nfd::FaceEventNotification& notification);
 
-private:
-  void
-  onCommandPrefixAddNextHopSuccess(const Name& prefix, const ControlParameters& result);
-
-  void
-  onCommandPrefixAddNextHopError(const Name& name, const ControlResponse& response);
-
-  void
-  onEnableLocalFieldsSuccess();
-
-  void
-  onEnableLocalFieldsError(const ControlResponse& response);
-
 private:
   Rib& m_rib;
   ndn::nfd::Controller& m_nfdController;

rib/rib.cpp

@@ -396,18 +396,27 @@ Rib::sendBatchFromQueue()
   // Until task #1698, each RibUpdateBatch contains exactly one RIB update
   BOOST_ASSERT(batch.size() == 1);
 
-  const Rib::UpdateSuccessCallback& managerSuccessCallback = item.managerSuccessCallback;
-  const Rib::UpdateFailureCallback& managerFailureCallback = item.managerFailureCallback;
+  auto fibSuccessCb = bind(&Rib::onFibUpdateSuccess, this, batch, _1, item.managerSuccessCallback);
+  auto fibFailureCb = bind(&Rib::onFibUpdateFailure, this, item.managerFailureCallback, _1, _2);
 
-  m_fibUpdater->computeAndSendFibUpdates(batch,
-                                         bind(&Rib::onFibUpdateSuccess, this,
-                                              batch, _1, managerSuccessCallback),
-                                         bind(&Rib::onFibUpdateFailure, this,
-                                              managerFailureCallback, _1, _2));
-
-  if (m_onSendBatchFromQueue != nullptr) {
-    m_onSendBatchFromQueue(batch);
+#ifdef WITH_TESTS
+  if (mockFibResponse != nullptr) {
+    m_fibUpdater->computeAndSendFibUpdates(batch, bind([]{}), bind([]{}));
+    bool shouldFibSucceed = mockFibResponse(batch);
+    if (wantMockFibResponseOnce) {
+      mockFibResponse = nullptr;
+    }
+    if (shouldFibSucceed) {
+      fibSuccessCb(m_fibUpdater->m_inheritedRoutes);
+    }
+    else {
+      fibFailureCb(504, "mocked failure");
+    }
+    return;
   }
+#endif
+
+  m_fibUpdater->computeAndSendFibUpdates(batch, fibSuccessCb, fibFailureCb);
 }
 
 void

rib/rib.hpp

@@ -175,8 +175,16 @@ private:
   sendBatchFromQueue();
 
 PUBLIC_WITH_TESTS_ELSE_PRIVATE:
-  // Used by RibManager unit-tests to get sent batch to simulate successful FIB update
-  std::function<void(RibUpdateBatch)> m_onSendBatchFromQueue;
+#ifdef WITH_TESTS
+  /** \brief In unit tests, mock FIB update result.
+   *
+   *  If the callback is not nullptr, sendBatchFromQueue() immediately succeeds or fails according
+   *  to the return value of callback function.
+   */
+  std::function<bool(const RibUpdateBatch&)> mockFibResponse;
+
+  bool wantMockFibResponseOnce = false; ///< if true, mockFibResponse is cleared after every use.
+#endif
 
   void
   erase(const Name& prefix, const Route& route);

tests/rib/fib-updates-common.hpp

@@ -1,6 +1,6 @@
 /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
-/**
- * Copyright (c) 2014-2017,  Regents of the University of California,
+/*
+ * Copyright (c) 2014-2018,  Regents of the University of California,
  *                           Arizona Board of Regents,
  *                           Colorado State University,
  *                           University Pierre & Marie Curie, Sorbonne University,
@@ -81,7 +81,8 @@ public:
           .setName(name)
           .setRoute(route);
 
-    simulateSuccessfulResponse(update);
+    simulateSuccessfulResponse();
+    rib.beginApplyUpdate(update, nullptr, nullptr);
   }
 
   void
@@ -95,53 +96,17 @@ public:
           .setName(name)
           .setRoute(route);
 
-    simulateSuccessfulResponse(update);
-  }
-
-  void
-  onSendBatchFromQueue(const RibUpdateBatch& batch)
-  {
-    Rib::UpdateSuccessCallback managerCallback = bind(&FibUpdatesFixture::onSuccess, this);
-
-    // Only receive callback after the first send
-    rib.m_onSendBatchFromQueue = nullptr;
-
-    rib.onFibUpdateSuccess(batch, fibUpdater.m_inheritedRoutes, managerCallback);
+    simulateSuccessfulResponse();
+    rib.beginApplyUpdate(update, nullptr, nullptr);
   }
 
   void
   destroyFace(uint64_t faceId)
   {
-    rib.m_onSendBatchFromQueue = bind(&FibUpdatesFixture::onSendBatchFromQueue, this, _1);
-
+    simulateSuccessfulResponse();
     rib.beginRemoveFace(faceId);
   }
 
-  void
-  simulateSuccessfulResponse(const RibUpdate& update)
-  {
-    Rib::UpdateSuccessCallback managerCallback = bind(&FibUpdatesFixture::onSuccess, this);
-
-    rib.beginApplyUpdate(update, managerCallback, nullptr);
-
-    RibUpdateBatch batch(update.getRoute().faceId);
-    batch.add(update);
-
-    // Simulate a successful response from NFD
-    rib.onFibUpdateSuccess(batch, fibUpdater.m_inheritedRoutes, managerCallback);
-  }
-
-  void
-  onSuccess()
-  {
-  }
-
-  void
-  onFailure()
-  {
-    BOOST_FAIL("FibUpdate failed");
-  }
-
   const FibUpdater::FibUpdateList&
   getFibUpdates()
   {
@@ -153,7 +118,6 @@ public:
     return fibUpdates;
   }
 
-
   FibUpdater::FibUpdateList
   getSortedFibUpdates()
   {
@@ -169,6 +133,14 @@ public:
     fibUpdater.m_updatesForNonBatchFaceId.clear();
   }
 
+private:
+  void
+  simulateSuccessfulResponse()
+  {
+    rib.mockFibResponse = [] (const RibUpdateBatch&) { return true; };
+    rib.wantMockFibResponseOnce = true;
+  }
+
 public:
   ndn::util::DummyClientFace face;
   ndn::nfd::Controller controller;

tests/rib/rib-manager.t.cpp

@@ -65,7 +65,11 @@ public:
     , m_fibUpdater(m_rib, m_nfdController)
     , m_manager(m_rib, m_face, m_nfdController, m_dispatcher)
   {
-    m_rib.m_onSendBatchFromQueue = bind(&RibManagerFixture::onSendBatchFromQueue, this, _1);
+    m_rib.mockFibResponse = [] (const RibUpdateBatch& batch) {
+      BOOST_CHECK(batch.begin() != batch.end());
+      return true;
+    };
+    m_rib.wantMockFibResponseOnce = false;
 
     if (m_status.isLocalhostConfigured) {
       m_manager.applyLocalhostConfig(getValidatorConfigSection(), "test");
@@ -150,17 +154,6 @@ public:
       .setOrigin(ndn::nfd::ROUTE_ORIGIN_NLSR);
   }
 
-  void
-  onSendBatchFromQueue(const RibUpdateBatch& batch)
-  {
-    BOOST_ASSERT(batch.begin() != batch.end());
-    RibUpdate update = *(batch.begin());
-
-    // Simulate a successful response from NFD
-    m_rib.onFibUpdateSuccess(batch, m_fibUpdater.m_inheritedRoutes,
-                             bind(&RibManager::onRibUpdateSuccess, &m_manager, update));
-  }
-
 public:
   enum class CheckCommandResult {
     OK,