[3329] D2ClientMgr::sendRequest now calls client's error handler

Altered D2ClientMgr::sendRequest to call the client's error handler if
in send mode and sendRequest fails. Prior to this it simply allowed any
sender exceptions to propagate.

src/lib/dhcpsrv/d2_client_mgr.cc

@@ -264,7 +264,7 @@ D2ClientMgr::stopSender() {
     }
 
     // If its not null, call stop.
-    if (name_change_sender_)  {
+    if (amSending()) {
         name_change_sender_->stopSending();
         LOG_INFO(dhcpsrv_logger, DHCPSRV_DHCP_DDNS_SENDER_STOPPED);
     }
@@ -272,11 +272,36 @@ D2ClientMgr::stopSender() {
 
 void
 D2ClientMgr::sendRequest(dhcp_ddns::NameChangeRequestPtr& ncr) {
-    if (!name_change_sender_) {
-        isc_throw(D2ClientError, "D2ClientMgr::sendRequest sender is null");
+    if (!amSending()) {
+        // This is programmatic error so bust them for it.
+        isc_throw(D2ClientError, "D2ClientMgr::sendRequest not in send mode");
     }
 
-    name_change_sender_->sendRequest(ncr);
+    try {
+        name_change_sender_->sendRequest(ncr);
+    } catch (const std::exception& ex) {
+        LOG_ERROR(dhcpsrv_logger, DHCPSRV_DHCP_DDNS_NCR_REJECTED)
+                  .arg(ex.what()).arg((ncr ? ncr->toText() : " NULL "));
+        invokeClientErrorHandler(dhcp_ddns::NameChangeSender::ERROR, ncr);
+    }
+}
+
+void
+D2ClientMgr::invokeClientErrorHandler(const dhcp_ddns::NameChangeSender::
+                                      Result result,
+                                      dhcp_ddns::NameChangeRequestPtr& ncr) {
+    // Handler is mandatory to enter send mode but test it just to be safe.
+    if (!client_error_handler_) {
+        LOG_ERROR(dhcpsrv_logger, DHCPSRV_DHCP_DDNS_HANDLER_NULL);
+    } else {
+        // Handler is not supposed to throw, but catch just in case.
+        try {
+            (client_error_handler_)(result, ncr);
+        } catch (const std::exception& ex) {
+            LOG_ERROR(dhcpsrv_logger, DHCPSRV_DHCP_DDNS_ERROR_EXCEPTION)
+                      .arg(ex.what());
+        }
+    }
 }
 
 size_t
@@ -288,6 +313,16 @@ D2ClientMgr::getQueueSize() const {
     return(name_change_sender_->getQueueSize());
 }
 
+size_t
+D2ClientMgr::getQueueMaxSize() const {
+    if (!name_change_sender_) {
+        isc_throw(D2ClientError, "D2ClientMgr::getQueueMaxSize sender is null");
+    }
+
+    return(name_change_sender_->getQueueMaxSize());
+}
+
+
 
 const dhcp_ddns::NameChangeRequestPtr&
 D2ClientMgr::peekAt(const size_t index) const {
@@ -314,21 +349,8 @@ D2ClientMgr::operator()(const dhcp_ddns::NameChangeSender::Result result,
         LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL,
                   DHCPSRV_DHCP_DDNS_NCR_SENT).arg(ncr->toText());
     } else {
-        // Handler is mandatory but test it just to be safe.
-        /// @todo Until we have a better feel for how errors need to be
-        /// handled we farm it out to the application layer.
-        if (client_error_handler_) {
-            // Handler is not supposed to throw, but catch just in case.
-            try {
-                (client_error_handler_)(result, ncr);
-            } catch (const std::exception& ex) {
-                LOG_ERROR(dhcpsrv_logger, DHCPSRV_DHCP_DDNS_ERROR_EXCEPTION)
-                          .arg(ex.what());
-            }
-        } else {
-            LOG_ERROR(dhcpsrv_logger, DHCPSRV_DHCP_DDNS_HANDLER_NULL);
-        }
-   }
+        invokeClientErrorHandler(result, ncr);
+    }
 }
 
 int

src/lib/dhcpsrv/d2_client_mgr.h

@@ -42,7 +42,7 @@ namespace dhcp {
 /// @param result Result code of the send operation.
 /// @param ncr NameChangeRequest which failed to send.
 ///
-/// @note Handlers are expected not to throw. In the event a hanlder does
+/// @note Handlers are expected not to throw. In the event a handler does
 /// throw invoking code logs the exception and then swallows it.
 typedef
 boost::function<void(const dhcp_ddns::NameChangeSender::Result result,
@@ -248,17 +248,36 @@ public:
     /// @brief Send the given NameChangeRequests to b10-dhcp-ddns
     ///
     /// Passes NameChangeRequests to the NCR sender for transmission to
-    /// b10-dhcp-ddns.
+    /// b10-dhcp-ddns. If the sender rejects the message, the client's error
+    /// handler will be invoked.  The most likely cause for rejection is
+    /// the senders' queue has reached maximum capacity.
     ///
     /// @param ncr NameChangeRequest to send
     ///
-    /// @throw D2ClientError if sender instance is null. Underlying layer
-    /// may throw NCRSenderExceptions exceptions.
+    /// @throw D2ClientError if sender instance is null or not in send
+    /// mode.  Either of these represents a programmatic error.
     void sendRequest(dhcp_ddns::NameChangeRequestPtr& ncr);
 
+    /// @brief Calls the client's error handler.
+    ///
+    /// Calls the error handler method set by startSender() when an
+    /// error occurs attempting to send a method.  If the error handler
+    /// throws an exception it will be caught and logged.
+    ///
+    /// @param result contains that send outcome status.
+    /// @param ncr is a pointer to the NameChangeRequest that was attempted.
+    ///
+    /// This method is exception safe.
+    void invokeClientErrorHandler(const dhcp_ddns::NameChangeSender::
+                                  Result result,
+                                  dhcp_ddns::NameChangeRequestPtr& ncr);
+
     /// @brief Returns the number of NCRs queued for transmission.
     size_t getQueueSize() const;
 
+    /// @brief Returns the maximum number of NCRs allowed in the queue.
+    size_t getQueueMaxSize() const;
+
     /// @brief Returns the nth NCR queued for transmission.
     ///
     /// Note that the entry is not removed from the queue.

src/lib/dhcpsrv/dhcpsrv_messages.mes

@@ -375,3 +375,9 @@ off.  This should only occur if IO errors communicating with b10-dhcp-ddns
 have been experienced.  Any such errors should have preceding entries in the
 log with details.  No further attempts to communicate with b10-dhcp-ddns will
 be made without intervention.
+
+% DHCPSRV_DHCP_DDNS_NCR_REJECTED NameChangeRequest rejected by the sender: %1, ncr: %2
+This is an error message indicating that NameChangeSender used to deliver DDNS
+update requests to b10-dhcp-ddns rejected the request.  This most likely cause
+is the sender's queue has reached maximum capacity.  This would imply that
+requests are being generated faster than they can be delivered.

src/lib/dhcpsrv/tests/d2_udp_unittest.cc

@@ -232,7 +232,7 @@ TEST_F(D2ClientMgrTest, udpSenderQueing) {
 
     // Trying to send a NCR when not in send mode should fail.
     dhcp_ddns::NameChangeRequestPtr ncr = buildTestNcr();
-    EXPECT_THROW(sendRequest(ncr), dhcp_ddns::NcrSenderError);
+    EXPECT_THROW(sendRequest(ncr), D2ClientError);
 
     // Place sender in send mode.
     ASSERT_NO_THROW(startSender(getErrorHandler()));
@@ -380,6 +380,7 @@ TEST_F(D2ClientMgrTest, udpSendErrorHandlerThrow) {
     ASSERT_EQ(0, error_handler_count_);
 }
 
+/// @brief Tests that D2ClientMgr registers and unregisters with IfaceMgr.
 TEST_F(D2ClientMgrTest, ifaceRegister) {
     // Enable DDNS with server at 127.0.0.1/prot 53001 via UDP.
     enableDdns("127.0.0.1", 530001, dhcp_ddns::NCR_UDP);
@@ -414,7 +415,7 @@ TEST_F(D2ClientMgrTest, ifaceRegister) {
     ASSERT_EQ(2, callback_count_);
     ASSERT_EQ(0, error_handler_count_);
 
-    // Calling recevie again should have no affect.
+    // Calling receive again should have no affect.
     IfaceMgr::instance().receive4(0, 0);
     EXPECT_EQ(1, getQueueSize());
     ASSERT_EQ(2, callback_count_);
@@ -453,4 +454,19 @@ TEST_F(D2ClientMgrTest, udpSuspendUpdates) {
     ASSERT_EQ(1, getQueueSize());
 }
 
+/// @brief Tests that invokeErrorHandler does not fail if there is no handler.
+TEST_F(D2ClientMgrTest, missingErrorHandler) {
+    // Ensure we aren't in send mode.
+    ASSERT_FALSE(ddnsEnabled());
+    ASSERT_FALSE(amSending());
+
+    // There is no error handler at this point, so invoking should not throw.
+    dhcp_ddns::NameChangeRequestPtr ncr;
+    ASSERT_NO_THROW(invokeClientErrorHandler(dhcp_ddns::NameChangeSender::ERROR,
+                                             ncr));
+
+    // Verify we didn't invoke the error handler, error count is zero.
+    ASSERT_EQ(0, error_handler_count_);
+}
+
 } // end of anonymous namespace