[3008] Addtional review changes. Added fail-safe try-catch blocks

around IO completion handler invocations and minor clean up.

src/bin/d2/d2_messages.mes

@@ -126,38 +126,42 @@ unrecoverable error from within the event loop.
 
 % DHCP_DDNS_INVALID_NCR application received an invalid DNS update request: %1
 This is an error message that indicates that an invalid request to update
-a DNS entry was recevied by the application.  Either the format or the content
-of the request is incorret. The request will be ignored.
+a DNS entry was received by the application.  Either the format or the content
+of the request is incorrect. The request will be ignored.
 
 % DHCP_DDNS_INVALID_RESPONSE received response to DNS Update message is malformed: %1
 This is a debug message issued when the DHCP-DDNS application encountered an
 error while decoding a response to DNS Update message. Typically, this error
 will be encountered when a response message is malformed.
 
-% DHCP_DDNS_NCR_LISTEN_CLOSE_ERROR application encountered an error while closing the the listener used to receive NameChangeRequests : %1
+% DHCP_DDNS_NCR_LISTEN_CLOSE_ERROR application encountered an error while closing the listener used to receive NameChangeRequests : %1
 This is an error message that indicates the application was unable to close the
 listener connection used to receive NameChangeRequests.  Closure may occur
-during the course of error recovery or duing normal shutdown procedure.  In
+during the course of error recovery or during normal shutdown procedure.  In
 either case the error is unlikely to impair the application's ability to
 process requests but it should be reported for analysis.
 
-% DHCP_DDNS_NCR_RECV_NEXT application could not initiate the next read following
-a request receive.
-This is a error message indicating that NameChangeRequest listener could not start another read after receiving a request.  While possible, this is highly unlikely and is probably a programmatic error.  The application should recover on its
-own.
-
-% DHCP_DDNS_NCR_SEND_CLOSE_ERROR DHCP-DDNS client encountered an error while closing ththe sender connection used to send NameChangeRequests : %1
-This is an error message that indicates the DHCP-DDNS client was unable to close
-the connection used to send NameChangeRequests.  Closure may occur during the
-course of error recovery or duing normal shutdown procedure.  In either case
-the error is unlikely to impair the client's ability to send requests but it
-should be reported for analysis.
-
-% DHCP_DDNS_NCR_SEND_NEXT DHCP-DDNS client could not initiate the next request send following send completion.
-This is a error message indicating that NameChangeRequest sender could not start another send after completing the send of the previous request.  While possible, this is highly unlikely and is probably a programmatic error.  The application should recover on its own.
+% DHCP_DDNS_NCR_RECV_NEXT_ERROR application could not initiate the next read following a request receive.
+This is a error message indicating that NameChangeRequest listener could not
+start another read after receiving a request.  While possible, this is highly
+unlikely and is probably a programmatic error.  The application should recover
+on its own.
+
+% DHCP_DDNS_NCR_SEND_CLOSE_ERROR DHCP-DDNS client encountered an error while closing the sender connection used to send NameChangeRequests : %1
+This is an error message that indicates the DHCP-DDNS client was unable to
+close the connection used to send NameChangeRequests.  Closure may occur during
+the course of error recovery or during normal shutdown procedure.  In either
+case the error is unlikely to impair the client's ability to send requests but
+it should be reported for analysis.
+
+% DHCP_DDNS_NCR_SEND_NEXT_ERROR DHCP-DDNS client could not initiate the next request send following send completion.
+This is a error message indicating that NameChangeRequest sender could not
+start another send after completing the send of the previous request.  While
+possible, this is highly unlikely and is probably a programmatic error.  The
+application should recover on its own.
 
 % DHCP_DDNS_NCR_UDP_RECV_ERROR UDP socket receive error while listening for DNS Update requests: %1
-This is an error message indicating that an IO error occured while listening
+This is an error message indicating that an IO error occurred while listening
 over a UDP socket for DNS update requests. This could indicate a network
 connectivity or system resource issue.
 
@@ -181,3 +185,15 @@ in event loop.
 % DHCP_DDNS_SHUTDOWN application is performing a normal shut down
 This is a debug message issued when the application has been instructed
 to shut down by the controller.
+
+% DHCP_DDNS_UNCAUGHT_NCR_RECV_HANDLER_ERROR unexpected exception thrown from the application receive completion handler: %1
+This is an error message that indicates that an exception was thrown but not
+caught in the application's request receive completion handler.  This is a
+programmatic error that needs to be reported.  Dependent upon the nature of
+the error the application may or may not continue operating normally.
+
+% DHCP_DDNS_UNCAUGHT_NCR_SEND_HANDLER_ERROR unexpected exception thrown from the DHCP-DDNS client send completion handler: %1
+This is an error message that indicates that an exception was thrown but not
+caught in the application's send completion handler.  This is a programmatic
+error that needs to be reported.  Dependent upon the nature of the error the
+client may or may not continue operating normally.

src/bin/d2/ncr_io.cc

@@ -72,7 +72,15 @@ void
 NameChangeListener::invokeRecvHandler(const Result result,
                                       NameChangeRequestPtr& ncr) {
     // Call the registered application layer handler.
-    recv_handler_(result, ncr);
+    // Surround the invocation with a try-catch. The invoked handler is
+    // not supposed to throw, but in the event it does we will at least
+    // report it.
+    try {
+        recv_handler_(result, ncr);
+    } catch (const std::exception& ex) {
+        LOG_ERROR(dctl_logger, DHCP_DDNS_UNCAUGHT_NCR_RECV_HANDLER_ERROR)
+                  .arg(ex.what());
+    }
 
     // Start the next IO layer asynchronous receive.
     // In the event the handler above intervened and decided to stop listening
@@ -87,9 +95,21 @@ NameChangeListener::invokeRecvHandler(const Result result,
             // at the IOService::run (or run variant) invocation.  So we will
             // close the window by invoking the application handler with
             // a failed result, and let the application layer sort it out.
-            LOG_ERROR(dctl_logger, DHCP_DDNS_NCR_RECV_NEXT).arg(ex.what());
+            LOG_ERROR(dctl_logger, DHCP_DDNS_NCR_RECV_NEXT_ERROR)
+                      .arg(ex.what());
+
+            // Call the registered application layer handler.
+            // Surround the invocation with a try-catch. The invoked handler is
+            // not supposed to throw, but in the event it does we will at least
+            // report it.
             NameChangeRequestPtr empty;
-            recv_handler_(ERROR, empty);
+            try {
+                recv_handler_(ERROR, empty);
+            } catch (const std::exception& ex) {
+                LOG_ERROR(dctl_logger,
+                          DHCP_DDNS_UNCAUGHT_NCR_RECV_HANDLER_ERROR)
+                          .arg(ex.what());
+            }
         }
     }
 }
@@ -201,7 +221,15 @@ NameChangeSender::invokeSendHandler(const NameChangeSender::Result result) {
 
     // Invoke the completion handler passing in the result and a pointer
     // the request involved.
-    send_handler_(result, ncr_to_send_);
+    // Surround the invocation with a try-catch. The invoked handler is
+    // not supposed to throw, but in the event it does we will at least
+    // report it.
+    try {
+        send_handler_(result, ncr_to_send_);
+    } catch (const std::exception& ex) {
+        LOG_ERROR(dctl_logger, DHCP_DDNS_UNCAUGHT_NCR_SEND_HANDLER_ERROR)
+                  .arg(ex.what());
+    }
 
     // Clear the pending ncr pointer.
     ncr_to_send_.reset();
@@ -216,8 +244,19 @@ NameChangeSender::invokeSendHandler(const NameChangeSender::Result result) {
         // at the IOService::run (or run variant) invocation.  So we will
         // close the window by invoking the application handler with
         // a failed result, and let the application layer sort it out.
-        LOG_ERROR(dctl_logger, DHCP_DDNS_NCR_SEND_NEXT).arg(ex.what());
-        send_handler_(ERROR, ncr_to_send_);
+        LOG_ERROR(dctl_logger, DHCP_DDNS_NCR_SEND_NEXT_ERROR)
+                  .arg(ex.what());
+
+        // Invoke the completion handler passing in failed result.
+        // Surround the invocation with a try-catch. The invoked handler is
+        // not supposed to throw, but in the event it does we will at least
+        // report it.
+        try {
+            send_handler_(ERROR, ncr_to_send_);
+        } catch (const std::exception& ex) {
+            LOG_ERROR(dctl_logger, DHCP_DDNS_UNCAUGHT_NCR_SEND_HANDLER_ERROR)
+                      .arg(ex.what());
+        }
     }
 }
 

src/bin/d2/ncr_io.h

@@ -216,12 +216,11 @@ public:
     /// exceptions at the handler level are application issues and should be
     /// dealt with at that level.
     ///
-    /// If the handler were to throw, the exception will surface at
-    /// IOService::run (or run variant) method invocation as this occurs as
-    /// part of the callback chain.  This will cause the invocation of
-    /// doReceive to be skipped which will break the listen-receive-listen
-    /// cycle. To restart the cycle it would be necessary to call
-    /// stopListener() and then startListener().
+    /// This method does wrap the handler invocation within a try-catch
+    /// block as a fail-safe.  The exception will be logged but the
+    /// receive logic will continue.  What this implies is that continued
+    /// operation may or may not succeed as the application has violated
+    /// the interface contract.
     ///
     /// @param result contains that receive outcome status.
     /// @param ncr is a pointer to the newly received NameChangeRequest if
@@ -434,7 +433,8 @@ public:
         /// delivered (or attempted).
         ///
         /// @throw This method MUST NOT throw.
-        virtual void operator ()(const Result result, NameChangeRequestPtr& ncr) = 0;
+        virtual void operator ()(const Result result,
+                                 NameChangeRequestPtr& ncr) = 0;
     };
 
     /// @brief Constructor
@@ -508,12 +508,11 @@ public:
     /// handler level are application issues and should be dealt with at that
     /// level.
     ///
-    /// If the handler were to throw, the exception will surface at
-    /// IOService::run (or run variant) method invocation as this occurs as
-    /// part of the callback chain.  This will cause the invocation of
-    /// sendNext to be skipped which will interrupt automatic buffer drain
-    /// cycle.  Assuming there is not a connectivity issue, the cycle will
-    /// resume with the next sendRequest call, or an explicit call to sendNext.
+    /// This method does wrap the handler invocation within a try-catch
+    /// block as a fail-safe.  The exception will be logged but the
+    /// send logic will continue.  What this implies is that continued
+    /// operation may or may not succeed as the application has violated
+    /// the interface contract.
     ///
     /// @param result contains that send outcome status.
     void invokeSendHandler(const NameChangeSender::Result result);

src/bin/d2/ncr_udp.cc

@@ -103,7 +103,7 @@ NameChangeUDPListener::open(isc::asiolink::IOService& io_service) {
                                   (ip_address_.isV4() ? asio::ip::udp::v4() :
                                    asio::ip::udp::v6())));
 
-        // If in test mode, enable address reuse.
+        // Set the socket option to reuse addresses if it is enabled.
         if (reuse_address_) {
             asio_socket_->set_option(asio::socket_base::reuse_address(true));
         }
@@ -182,11 +182,11 @@ NameChangeUDPListener::receiveCompletionHandler(const bool successful,
 //*************************** NameChangeUDPSender ***********************
 
 NameChangeUDPSender::
-NameChangeUDPSender(const isc::asiolink::IOAddress& ip_address, 
-                    const uint32_t port, 
+NameChangeUDPSender(const isc::asiolink::IOAddress& ip_address,
+                    const uint32_t port,
                     const isc::asiolink::IOAddress& server_address,
                     const uint32_t server_port, const NameChangeFormat format,
-                    RequestSendHandler& ncr_send_handler, 
+                    RequestSendHandler& ncr_send_handler,
                     const size_t send_que_max, const bool reuse_address)
     : NameChangeSender(ncr_send_handler, send_que_max),
       ip_address_(ip_address), port_(port), server_address_(server_address),
@@ -220,7 +220,7 @@ NameChangeUDPSender::open(isc::asiolink::IOService& io_service) {
                                   (ip_address_.isV4() ? asio::ip::udp::v4() :
                                    asio::ip::udp::v6())));
 
-        // If in test mode, enable address reuse.
+        // Set the socket option to reuse addresses if it is enabled.
         if (reuse_address_) {
             asio_socket_->set_option(asio::socket_base::reuse_address(true));
         }

src/bin/d2/ncr_udp.h

@@ -168,7 +168,7 @@ public:
         /// send.
         /// @param buf_size is the capacity of the buffer
         /// @param data_source storage for UDP endpoint which supplied the data
-        Data(RawBufferPtr &buffer, const size_t buf_size, 
+        Data(RawBufferPtr& buffer, const size_t buf_size,
              UDPEndpointPtr& data_source)
             : buffer_(buffer), buf_size_(buf_size), data_source_(data_source),
               put_len_(0), error_code_(), bytes_transferred_(0) {
@@ -225,7 +225,7 @@ public:
     /// buffer.  For a write it is the number of bytes read from the
     /// buffer.
     void operator ()(const asio::error_code error_code,
-                             const size_t bytes_transferred);
+                     const size_t bytes_transferred);
 
     /// @brief Returns the number of bytes transferred by the completed IO
     /// service.
@@ -295,12 +295,12 @@ public:
     /// @brief Sets the data source to the given endpoint.
     ///
     /// @param endpoint is the new value to assign to data source.
-    void setDataSource(UDPEndpointPtr endpoint) {
+    void setDataSource(UDPEndpointPtr& endpoint) {
         data_->data_source_ = endpoint;
     }
 
     /// @brief Returns the UDP endpoint that provided the transferred data.
-    UDPEndpointPtr getDataSource() {
+    const UDPEndpointPtr& getDataSource() {
         return (data_->data_source_);
     }
 
@@ -421,8 +421,7 @@ private:
     /// @brief Pointer to the receive callback
     boost::shared_ptr<UDPCallback> recv_callback_;
 
-    /// @brief indicator that signifies listener is being used
-    /// in test mode
+    /// @brief Flag which enables the reuse address socket option if true.
     bool reuse_address_;
 
     ///
@@ -484,7 +483,7 @@ public:
     /// @param io_service the IOService which will monitor the socket.
     ///
     /// @throw NcrUDPError if the open fails.
-    virtual void open(isc::asiolink::IOService & io_service);
+    virtual void open(isc::asiolink::IOService& io_service);
 
 
     /// @brief Closes the UDPSocket.
@@ -553,8 +552,7 @@ private:
     /// @brief Pointer to the send callback
     boost::shared_ptr<UDPCallback> send_callback_;
 
-    /// @brief boolean indicator that signifies sender is being used
-    /// in test mode
+    /// @brief Flag which enables the reuse address socket option if true.
     bool reuse_address_;
 };