[3008] Addressed review comments.

src/bin/d2/d2_messages.mes

@@ -134,24 +134,32 @@ 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_UDP_LISTEN_CLOSE application encountered an error while closing the UDP socket used to receive NameChangeRequests : %1
+% DHCP_DDNS_NCR_LISTEN_CLOSE_ERROR application encountered an error while closing the the listener used to receive NameChangeRequests : %1
 This is an error message that indicates the application was unable to close the
-UDP socket being used to receive NameChangeRequests.  Socket closure may occur
+listener connection used to receive 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 application's ability to
 process requests but it should be reported for analysis.
 
-% 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
-over a UDP socket for DNS update requests. in the request. This could indicate
-a network connectivity or system resource issue.
+% 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_UDP_SEND_CLOSE application encountered an error while closing the UDP socket used to send NameChangeRequests : %1
+% 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 UDP socket being used to send NameChangeRequests.  Socket 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.
+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_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
+over a UDP socket for DNS update requests. This could indicate a network
+connectivity or system resource issue.
 
 % DHCP_DDNS_NO_MATCH No DNS servers match FQDN: %1
 This is warning message issued when there are no domains in the configuration

src/bin/d2/ncr_io.cc

@@ -20,13 +20,9 @@ namespace d2 {
 
 //************************** NameChangeListener ***************************
 
-NameChangeListener::NameChangeListener(const RequestReceiveHandler*
+NameChangeListener::NameChangeListener(RequestReceiveHandler&
                                        recv_handler)
     : listening_(false), recv_handler_(recv_handler) {
-    if (!recv_handler) {
-        isc_throw(NcrListenerError,
-                      "NameChangeListener ctor - recv_handler cannot be null");
-    }
 };
 
 void
@@ -64,41 +60,64 @@ NameChangeListener::stopListening() {
     } catch (const isc::Exception &ex) {
         // Swallow exceptions. If we have some sort of error we'll log
         // it but we won't propagate the throw.
-        LOG_ERROR(dctl_logger, DHCP_DDNS_NCR_UDP_LISTEN_CLOSE).arg(ex.what());
+        LOG_ERROR(dctl_logger, DHCP_DDNS_NCR_LISTEN_CLOSE_ERROR).arg(ex.what());
     }
 
+    // Set it false, no matter what.  This allows us to at least try to
+    // re-open via startListening().
     setListening(false);
 }
 
 void
+#if 0
 NameChangeListener::invokeRecvHandler(Result result, NameChangeRequestPtr ncr) {
+#else
+NameChangeListener::invokeRecvHandler(const Result result, 
+                                      NameChangeRequestPtr& ncr) {
+#endif
     // Call the registered application layer handler.
-    (*(const_cast<RequestReceiveHandler*>(recv_handler_)))(result, ncr);
+    recv_handler_(result, ncr);
 
     // Start the next IO layer asynchronous receive.
-    doReceive();
-};
+    // In the event the handler above intervened and decided to stop listening
+    // we need to check that first.
+    if (amListening()) {
+        try {
+            doReceive();
+        } catch (const isc::Exception& ex) {
+            // It is possible though unlikely, for doReceive to fail without
+            // scheduling the read. While, unlikely, it does mean the callback
+            // will not get called with a failure. A throw here would surface
+            // 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());
+#if 0
+            recv_handler_(ERROR, NameChangeRequestPtr());
+#else
+            NameChangeRequestPtr empty;
+            recv_handler_(ERROR, empty);
+#endif
+        }
+    }
+}
 
 //************************* NameChangeSender ******************************
 
-NameChangeSender::NameChangeSender(const RequestSendHandler* send_handler,
-                                   size_t send_que_max)
+NameChangeSender::NameChangeSender(RequestSendHandler& send_handler,
+                                   size_t send_queue_max)
     : sending_(false), send_handler_(send_handler),
-      send_que_max_(send_que_max) {
-    if (!send_handler) {
-        isc_throw(NcrSenderError,
-                  "NameChangeSender ctor - send_handler cannot be null");
-    }
+      send_queue_max_(send_queue_max) {
 
     // Queue size must be big enough to hold at least 1 entry.
-    if (send_que_max == 0) {
-        isc_throw(NcrSenderError, "NameChangeSender ctor -"
+    if (send_queue_max == 0) {
+        isc_throw(NcrSenderError, "NameChangeSender constructor"
                   " queue size must be greater than zero");
     }
-};
+}
 
 void
-NameChangeSender::startSending(isc::asiolink::IOService & io_service) {
+NameChangeSender::startSending(isc::asiolink::IOService& io_service) {
     if (amSending()) {
         // This amounts to a programmatic error.
         isc_throw(NcrSenderError, "NameChangeSender is already sending");
@@ -112,7 +131,7 @@ NameChangeSender::startSending(isc::asiolink::IOService & io_service) {
         open(io_service);
     } catch (const isc::Exception& ex) {
         stopSending();
-        isc_throw(NcrSenderOpenError, "Open failed:" << ex.what());
+        isc_throw(NcrSenderOpenError, "Open failed: " << ex.what());
     }
 
     // Set our status to sending.
@@ -127,13 +146,16 @@ NameChangeSender::stopSending() {
     } catch (const isc::Exception &ex) {
         // Swallow exceptions. If we have some sort of error we'll log
         // it but we won't propagate the throw.
-        LOG_ERROR(dctl_logger, DHCP_DDNS_NCR_UDP_SEND_CLOSE).arg(ex.what());
+        LOG_ERROR(dctl_logger, DHCP_DDNS_NCR_SEND_CLOSE_ERROR).arg(ex.what());
     }
+
+    // Set it false, no matter what.  This allows us to at least try to
+    // re-open via startSending().
     setSending(false);
 }
 
 void
-NameChangeSender::sendRequest(NameChangeRequestPtr ncr) {
+NameChangeSender::sendRequest(NameChangeRequestPtr& ncr) {
     if (!amSending()) {
         isc_throw(NcrSenderError, "sender is not ready to send");
     }
@@ -142,13 +164,13 @@ NameChangeSender::sendRequest(NameChangeRequestPtr ncr) {
         isc_throw(NcrSenderError, "request to send is empty");
     }
 
-    if (send_que_.size() >= send_que_max_) {
-        isc_throw(NcrSenderQueFull, "send queue has reached maximum capacity:"
-                  << send_que_max_ );
+    if (send_queue_.size() >= send_queue_max_) {
+        isc_throw(NcrSenderQueueFull, "send queue has reached maximum capacity:"
+                  << send_queue_max_ );
     }
 
     // Put it on the queue.
-    send_que_.push_back(ncr);
+    send_queue_.push_back(ncr);
 
     // Call sendNext to schedule the next one to go.
     sendNext();
@@ -165,8 +187,8 @@ NameChangeSender::sendNext() {
 
     // If queue isn't empty, then get one from the front. Note we leave
     // it on the front of the queue until we successfully send it.
-    if (send_que_.size()) {
-        ncr_to_send_ = send_que_.front();
+    if (send_queue_.size()) {
+        ncr_to_send_ = send_queue_.front();
 
        // @todo start defense timer
        // If a send were to hang and we timed it out, then timeout
@@ -178,39 +200,50 @@ NameChangeSender::sendNext() {
 }
 
 void
-NameChangeSender::invokeSendHandler(NameChangeSender::Result result) {
+NameChangeSender::invokeSendHandler(const NameChangeSender::Result result) {
     // @todo reset defense timer
     if (result == SUCCESS) {
         // It shipped so pull it off the queue.
-        send_que_.pop_front();
+        send_queue_.pop_front();
     }
 
     // Invoke the completion handler passing in the result and a pointer
     // the request involved.
-    (*(const_cast<RequestSendHandler*>(send_handler_))) (result, ncr_to_send_);
+    send_handler_(result, ncr_to_send_);
 
     // Clear the pending ncr pointer.
     ncr_to_send_.reset();
 
     // Set up the next send
-    sendNext();
-};
+    try {
+        sendNext();
+    } catch (const isc::Exception& ex) {
+        // It is possible though unlikely, for sendNext to fail without
+        // scheduling the send. While, unlikely, it does mean the callback
+        // will not get called with a failure. A throw here would surface
+        // 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_);
+    }
+}
 
 void
 NameChangeSender::skipNext() {
-    if (send_que_.size()) {
+    if (send_queue_.size()) {
         // Discards the request at the front of the queue.
-        send_que_.pop_front();
+        send_queue_.pop_front();
     }
 }
 
 void
-NameChangeSender::flushSendQue() {
+NameChangeSender::clearSendQueue() {
     if (amSending()) {
-        isc_throw(NcrSenderError, "Cannot flush queue while sending");
+        isc_throw(NcrSenderError, "Cannot clear queue while sending");
     }
 
-    send_que_.clear();
+    send_queue_.clear();
 }
 
 } // namespace isc::d2

src/bin/d2/ncr_io.h

@@ -19,28 +19,29 @@
 /// @brief This file defines abstract classes for exchanging NameChangeRequests.
 ///
 /// These classes are used for sending and receiving requests to update DNS
-/// information for FQDNs embodied as NameChangeRequests (aka NCRs). Ultimately,/// NCRs must move through the following three layers in order to implement
+/// information for FQDNs embodied as NameChangeRequests (aka NCRs). Ultimately,
+/// NCRs must move through the following three layers in order to implement
 /// DHCP-DDNS:
 ///
 ///    * Application layer - the business layer which needs to
 ///    transport NameChangeRequests, and is unaware of the means by which
 ///    they are transported.
 ///
-///    * IO layer - the low-level layer that is directly responsible for
-///    sending and receiving data asynchronously and is supplied through
-///    other libraries.  This layer is largely unaware of the nature of the
-///    data being transmitted.  In other words, it doesn't know beans about
-///    NCRs.
-///
 ///    * NameChangeRequest layer - This is the layer which acts as the
 ///    intermediary between the Application layer and the IO layer.  It must
 ///    be able to move NameChangeRequests to the IO layer as raw data and move
 ///    raw data from the IO layer in the Application layer as
 ///    NameChangeRequests.
 ///
-/// The abstract classes defined here implement implement the latter, middle
-/// layer, the NameChangeRequest layer.  There are two types of participants
-/// in this middle ground:
+///    * IO layer - the low-level layer that is directly responsible for
+///    sending and receiving data asynchronously and is supplied through
+///    other libraries.  This layer is largely unaware of the nature of the
+///    data being transmitted.  In other words, it doesn't know beans about
+///    NCRs.
+///
+/// The abstract classes defined here implement the latter, middle layer, 
+/// the NameChangeRequest layer.  There are two types of participants in this
+/// middle ground:
 ///
 ///    * listeners - Receive NCRs from one or more sources. The DHCP-DDNS
 ///   application, (aka D2), is a listener. Listeners are embodied by the
@@ -65,7 +66,7 @@
 namespace isc {
 namespace d2 {
 
-/// @brief Exception thrown if an NcrListenerError occurs.
+/// @brief Exception thrown if an NcrListenerError encounters a general error.
 class NcrListenerError : public isc::Exception {
 public:
     NcrListenerError(const char* file, size_t line, const char* what) :
@@ -168,16 +169,15 @@ public:
         /// result is NameChangeListener::SUCCESS.  It is indeterminate other
         /// wise.
         /// @throw This method MUST NOT throw.
-        virtual void operator ()(Result result, NameChangeRequestPtr ncr) = 0;
+        virtual void operator ()(const Result result, 
+                                 NameChangeRequestPtr& ncr) = 0;
     };
 
     /// @brief Constructor
     ///
     /// @param recv_handler is a pointer the application layer handler to be
     /// invoked each time a NCR is received or a receive error occurs.
-    ///
-    /// @throw throws NcrListenerError if recv_handler is NULL.
-    NameChangeListener(const RequestReceiveHandler* recv_handler);
+    NameChangeListener(RequestReceiveHandler& recv_handler);
 
     /// @brief Destructor
     virtual ~NameChangeListener() {
@@ -207,11 +207,27 @@ public:
     /// is called.  This method MUST be invoked by the derivation's
     /// implementation of doReceive.
     ///
+    /// NOTE:
+    /// The handler invoked by this method MUST NOT THROW. The handler is
+    /// at application level and should trap and handle any errors at
+    /// that level, rather than throw exceptions.  If an error has occurred
+    /// prior to invoking the handler, it will be expressed in terms a failed
+    /// result being passed to the handler, not a throw.  Therefore any 
+    /// 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().
+    ///
     /// @param result contains that receive outcome status.
     /// @param ncr is a pointer to the newly received NameChangeRequest if
     /// result is NameChangeListener::SUCCESS.  It is indeterminate other
     /// wise.
-    void invokeRecvHandler(Result result, NameChangeRequestPtr ncr);
+    void invokeRecvHandler(const Result result, NameChangeRequestPtr& ncr);
 
     /// @brief Abstract method which opens the IO source for reception.
     ///
@@ -266,7 +282,7 @@ private:
     bool listening_;
 
     /// @brief Application level NCR receive completion handler.
-    const RequestReceiveHandler* recv_handler_;
+    RequestReceiveHandler& recv_handler_;
 };
 
 /// @brief Defines a smart pointer to an instance of a listener.
@@ -288,9 +304,9 @@ public:
 };
 
 /// @brief Exception thrown if an error occurs initiating an IO send.
-class NcrSenderQueFull : public isc::Exception {
+class NcrSenderQueueFull : public isc::Exception {
 public:
-    NcrSenderQueFull(const char* file, size_t line, const char* what) :
+    NcrSenderQueueFull(const char* file, size_t line, const char* what) :
         isc::Exception(file, line, what) { };
 };
 
@@ -324,7 +340,7 @@ public:
 /// will remain there until one of three things occur:
 ///     * It is successfully delivered
 ///     * @c NameChangeSender::skipNext() is called
-///     * @c NameChangeSender::flushSendQue() is called
+///     * @c NameChangeSender::clearSendQueue() is called
 ///
 /// The queue contents are preserved across start and stop listening
 /// transitions. This is to provide for error recovery without losing
@@ -387,10 +403,10 @@ class NameChangeSender {
 public:
 
     /// @brief Defines the type used for the request send queue.
-    typedef std::deque<NameChangeRequestPtr> SendQue;
+    typedef std::deque<NameChangeRequestPtr> SendQueue;
 
     /// @brief Defines a default maximum number of entries in the send queue.
-    static const size_t MAX_QUE_DEFAULT = 1024;
+    static const size_t MAX_QUEUE_DEFAULT = 1024;
 
     /// @brief Defines the outcome of an asynchronous NCR send.
     enum Result {
@@ -418,20 +434,18 @@ public:
         /// delivered (or attempted).
         ///
         /// @throw This method MUST NOT throw.
-        virtual void operator ()(Result result, NameChangeRequestPtr ncr) = 0;
+        virtual void operator ()(const Result result, NameChangeRequestPtr& ncr) = 0;
     };
 
     /// @brief Constructor
     ///
     /// @param send_handler is a pointer the application layer handler to be
     /// invoked each time a NCR send attempt completes.
-    /// @param send_que_max is the maximum number of entries allowed in the
+    /// @param send_queue_max is the maximum number of entries allowed in the
     /// send queue.  Once the maximum number is reached, all calls to
     /// sendRequest will fail with an exception.
-    ///
-    /// @throw throws NcrSenderError if send_handler is NULL.
-    NameChangeSender(const RequestSendHandler* send_handler,
-            size_t send_que_max = MAX_QUE_DEFAULT);
+    NameChangeSender(RequestSendHandler& send_handler,
+            size_t send_queue_max = MAX_QUEUE_DEFAULT);
 
     /// @brief Destructor
     virtual ~NameChangeSender() {
@@ -458,13 +472,14 @@ public:
     ///
     /// The given request is placed at the back of the send queue and then
     /// sendNext is invoked.
+
     ///
     /// @param ncr is the NameChangeRequest to send.
     ///
     /// @throw throws NcrSenderError if the sender is not in sending state or
-    /// the request is empty; NcrSenderQueFull if the send queue has reached
+    /// the request is empty; NcrSenderQueueFull if the send queue has reached
     /// capacity.
-    void sendRequest(NameChangeRequestPtr ncr);
+    void sendRequest(NameChangeRequestPtr& ncr);
 
     /// @brief Dequeues and sends the next request on the send queue.
     ///
@@ -484,8 +499,24 @@ public:
     /// If not we leave it there so we can retry it.  After we invoke the
     /// handler we clear the pending ncr value and queue up the next send.
     ///
+    /// NOTE:
+    /// The handler invoked by this method MUST NOT THROW. The handler is
+    /// application level logic and should trap and handle any errors at
+    /// that level, rather than throw exceptions.  If IO errors have occurred
+    /// prior to invoking the handler, they are expressed in terms a failed
+    /// result being passed to the handler.  Therefore any 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  
+    /// 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.
+    ///
     /// @param result contains that send outcome status.
-    void invokeSendHandler(NameChangeSender::Result result);
+    void invokeSendHandler(const NameChangeSender::Result result);
 
     /// @brief Removes the request at the front of the send queue
     ///
@@ -502,11 +533,11 @@ public:
 
     /// @brief Flushes all entries in the send queue
     ///
-    /// This method can be used to flush all of the NCRs currently in the
+    /// This method can be used to discard all of the NCRs currently in the
     /// the send queue.  Note it may not be called while the sender is in
-    /// the sending state.
+    /// the sending state. 
     /// @throw throws NcrSenderError if called and sender is in sending state.
-    void flushSendQue();
+    void clearSendQueue();
 
     /// @brief Abstract method which opens the IO sink for transmission.
     ///
@@ -556,23 +587,14 @@ public:
         return ((ncr_to_send_) ? true : false);
     }
 
-    /// @brief Returns the request that is in the process of being sent.
-    ///
-    /// The pointer returned by this method will be populated with the
-    /// request that has been passed into doSend() and for which the
-    /// completion callback has not yet been invoked.
-    const NameChangeRequestPtr& getNcrToSend() {
-        return (ncr_to_send_);
-    }
-
     /// @brief Returns the maximum number of entries allowed in the send queue.
-    size_t getQueMaxSize() const  {
-        return (send_que_max_);
+    size_t getQueueMaxSize() const  {
+        return (send_queue_max_);
     }
 
     /// @brief Returns the number of entries currently in the send queue.
-    size_t getQueSize() const {
-        return (send_que_.size());
+    size_t getQueueSize() const {
+        return (send_queue_.size());
     }
 
 private:
@@ -590,13 +612,13 @@ private:
     bool sending_;
 
     /// @brief A pointer to regisetered send completion handler.
-    const RequestSendHandler* send_handler_;
+    RequestSendHandler& send_handler_;
 
     /// @brief Maximum number of entries permitted in the send queue.
-    size_t send_que_max_;
+    size_t send_queue_max_;
 
     /// @brief Queue of the requests waiting to be sent.
-    SendQue send_que_;
+    SendQueue send_queue_;
 
     /// @brief Pointer to the request which is in the process of being sent.
     NameChangeRequestPtr ncr_to_send_;

src/bin/d2/ncr_udp.cc

@@ -22,7 +22,6 @@
 namespace isc {
 namespace d2 {
 
-
 //*************************** UDPCallback ***********************
 
 UDPCallback::UDPCallback (RawBufferPtr buffer, size_t buf_size,
@@ -40,7 +39,7 @@ UDPCallback::UDPCallback (RawBufferPtr buffer, size_t buf_size,
 
 void
 UDPCallback::operator ()(const asio::error_code error_code,
-                             const size_t bytes_transferred) {
+                         const size_t bytes_transferred) {
 
     // Save the result state and number of bytes transferred.
     setErrorCode(error_code);
@@ -70,25 +69,23 @@ UDPCallback::putData(const uint8_t* src, size_t len) {
 
 
 //*************************** NameChangeUDPListener ***********************
-
-NameChangeUDPListener::NameChangeUDPListener(
-            const isc::asiolink::IOAddress& ip_address, const uint32_t port,
-            NameChangeFormat format,
-            const RequestReceiveHandler* ncr_recv_handler,
-            const bool reuse_address)
+NameChangeUDPListener::
+NameChangeUDPListener(const isc::asiolink::IOAddress& ip_address, 
+                      const uint32_t port, NameChangeFormat format, 
+                      RequestReceiveHandler& ncr_recv_handler,
+                      const bool reuse_address)
     : NameChangeListener(ncr_recv_handler), ip_address_(ip_address),
       port_(port), format_(format), reuse_address_(reuse_address) {
     // Instantiate the receive callback.  This gets passed into each receive.
     // Note that the callback constructor is passed an instance method
     // pointer to our recv_completion_handler.
-    recv_callback_.reset(new UDPCallback(
-                                       RawBufferPtr(new uint8_t[RECV_BUF_MAX]),
-                                       RECV_BUF_MAX,
-                                       UDPEndpointPtr(new asiolink::
-                                                      UDPEndpoint()),
-                                       boost::bind(&NameChangeUDPListener::
-                                       recv_completion_handler, this,
-                                       _1, _2)));
+    recv_callback_.reset(new 
+                         UDPCallback(RawBufferPtr(new uint8_t[RECV_BUF_MAX]),
+                                     RECV_BUF_MAX,
+                                     UDPEndpointPtr(new 
+                                                    asiolink::UDPEndpoint()),
+                                     boost::bind(&NameChangeUDPListener::
+                                     recv_completion_handler, this, _1, _2)));
 }
 
 NameChangeUDPListener::~NameChangeUDPListener() {
@@ -113,7 +110,7 @@ NameChangeUDPListener::open(isc::asiolink::IOService& io_service) {
             asio_socket_->set_option(asio::socket_base::reuse_address(true));
         }
 
-        // Bind the low leve socket to our endpoint.
+        // Bind the low level socket to our endpoint.
         asio_socket_->bind(endpoint.getASIOEndpoint());
     } catch (asio::system_error& ex) {
         isc_throw (NcrUDPError, ex.code().message());
@@ -190,7 +187,7 @@ NameChangeUDPSender::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, const size_t send_que_max,
+            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),

src/bin/d2/ncr_udp.h

@@ -341,7 +341,11 @@ public:
     NameChangeUDPListener(const isc::asiolink::IOAddress& ip_address,
                           const uint32_t port,
                           const NameChangeFormat format,
+#if 0
                           const RequestReceiveHandler* ncr_recv_handler,
+#else
+                          RequestReceiveHandler& ncr_recv_handler,
+#endif
                           const bool reuse_address = false);
 
     /// @brief Destructor.
@@ -460,16 +464,15 @@ public:
     /// when a send completes.
     /// @param send_que_max sets the maximum number of entries allowed in
     /// the send queue.
-    /// It defaults to NameChangeSender::MAX_QUE_DEFAULT
+    /// It defaults to NameChangeSender::MAX_QUEUE_DEFAULT
     /// @param reuse_address enables IP address sharing when true
     /// It defaults to false.
     ///
-    /// @throw base class throws NcrSenderError if handler is invalid.
     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,
-        const size_t send_que_max = NameChangeSender::MAX_QUE_DEFAULT,
+        RequestSendHandler& ncr_send_handler,
+        const size_t send_que_max = NameChangeSender::MAX_QUEUE_DEFAULT,
         const bool reuse_address = false);
 
     /// @brief Destructor

src/bin/d2/tests/ncr_udp_unittests.cc

@@ -75,29 +75,23 @@ const long TEST_TIMEOUT = 5 * 1000;
 /// @brief A NOP derivation for constructor test purposes.
 class SimpleListenHandler : public NameChangeListener::RequestReceiveHandler {
 public:
-    virtual void operator ()(NameChangeListener::Result, NameChangeRequestPtr) {
+    virtual void operator ()(const NameChangeListener::Result, 
+                             NameChangeRequestPtr&) {
     }
 };
 
 /// @brief Tests the NameChangeUDPListener constructors.
 /// This test verifies that:
-/// 1. Listener constructor requires valid completion handler
-/// 2. Given valid parameters, the listener constructor works
+/// 1. Given valid parameters, the listener constructor works
 TEST(NameChangeUDPListenerBasicTest, constructionTests) {
     // Verify the default constructor works.
     isc::asiolink::IOAddress ip_address(TEST_ADDRESS);
     uint32_t port = LISTENER_PORT;
     isc::asiolink::IOService io_service;
     SimpleListenHandler ncr_handler;
-
-    // Verify that constructing with an empty receive handler is not allowed.
-    EXPECT_THROW(NameChangeUDPListener(ip_address, port, FMT_JSON, NULL),
-                                       NcrListenerError);
-
     // Verify that valid constructor works.
     EXPECT_NO_THROW(NameChangeUDPListener(ip_address, port, FMT_JSON,
-                                          &ncr_handler));
-
+                                          ncr_handler));
 }
 
 /// @brief Tests NameChangeUDPListener starting and stopping listening .
@@ -115,7 +109,7 @@ TEST(NameChangeUDPListenerBasicTest, basicListenTests) {
 
     NameChangeListenerPtr listener;
     ASSERT_NO_THROW(listener.reset(
-        new NameChangeUDPListener(ip_address, port, FMT_JSON, &ncr_handler)));
+        new NameChangeUDPListener(ip_address, port, FMT_JSON, ncr_handler)));
 
     // Verify that we can start listening.
     EXPECT_NO_THROW(listener->startListening(io_service));
@@ -165,7 +159,7 @@ public:
           test_timer_(io_service_) {
         isc::asiolink::IOAddress addr(TEST_ADDRESS);
         listener_ = new NameChangeUDPListener(addr, LISTENER_PORT,
-                                              FMT_JSON, this, true);
+                                              FMT_JSON, *this, true);
 
         // Set the test timeout to break any running tasks if they hang.
         test_timer_.setup(boost::bind(&NameChangeUDPListenerTest::
@@ -206,8 +200,8 @@ public:
     /// The fixture acts as the "application" layer.  It derives from
     /// RequestReceiveHandler and as such implements operator() in order to
     /// receive NCRs.
-    virtual void operator ()(NameChangeListener::Result result,
-                             NameChangeRequestPtr ncr) {
+    virtual void operator ()(const NameChangeListener::Result result,
+                             NameChangeRequestPtr& ncr) {
         // save the result and the NCR we received
         result_ = result;
         received_ncr_ = ncr;
@@ -256,42 +250,43 @@ TEST_F(NameChangeUDPListenerTest, basicReceivetest) {
 /// @brief A NOP derivation for constructor test purposes.
 class SimpleSendHandler : public NameChangeSender::RequestSendHandler {
 public:
-    virtual void operator ()(NameChangeSender::Result, NameChangeRequestPtr) {
+    virtual void operator ()(const NameChangeSender::Result, 
+                             NameChangeRequestPtr&) {
     }
 };
 
 /// @brief Tests the NameChangeUDPSender constructors.
 /// This test verifies that:
-/// 1. Sender constructor requires valid completion handler
+/// 1. Constructing with a max queue size of 0 is not allowed
 /// 2. Given valid parameters, the sender constructor works
+/// 3. Default construction provides default max queue size 
+/// 4. Construction with a custom max queue size works
 TEST(NameChangeUDPSenderBasicTest, constructionTests) {
     isc::asiolink::IOAddress ip_address(TEST_ADDRESS);
     uint32_t port = SENDER_PORT;
     isc::asiolink::IOService io_service;
     SimpleSendHandler ncr_handler;
 
-    // Verify that constructing with an empty send handler is not allowed.
-    EXPECT_THROW(NameChangeUDPSender(ip_address, port,
-        ip_address, port, FMT_JSON, NULL), NcrSenderError);
-
     // Verify that constructing with an queue size of zero is not allowed.
     EXPECT_THROW(NameChangeUDPSender(ip_address, port,
-        ip_address, port, FMT_JSON, &ncr_handler, 0), NcrSenderError);
+        ip_address, port, FMT_JSON, ncr_handler, 0), NcrSenderError);
 
     NameChangeSenderPtr sender;
     // Verify that valid constructor works.
     EXPECT_NO_THROW(sender.reset(
                     new NameChangeUDPSender(ip_address, port, ip_address, port,
-                                            FMT_JSON, &ncr_handler)));
+                                            FMT_JSON, ncr_handler)));
 
     // Verify that send queue default max is correct.
-    size_t expected = NameChangeSender::MAX_QUE_DEFAULT;
-    EXPECT_EQ(expected, sender->getQueMaxSize());
+    size_t expected = NameChangeSender::MAX_QUEUE_DEFAULT;
+    EXPECT_EQ(expected, sender->getQueueMaxSize());
 
     // Verify that constructor with a valid custom queue size works.
     EXPECT_NO_THROW(sender.reset(
                     new NameChangeUDPSender(ip_address, port, ip_address, port,
-                                            FMT_JSON, &ncr_handler, 100)));
+                                            FMT_JSON, ncr_handler, 100)));
+
+    EXPECT_EQ(100, sender->getQueueMaxSize());
 }
 
 /// @brief Tests NameChangeUDPSender basic send functionality
@@ -308,7 +303,7 @@ TEST(NameChangeUDPSenderBasicTest, basicSendTests) {
     // Create the sender, setting the queue max equal to the number of
     // messages we will have in the list.
     NameChangeUDPSender sender(ip_address, port, ip_address, port,
-                               FMT_JSON, &ncr_handler, num_msgs);
+                               FMT_JSON, ncr_handler, num_msgs);
 
     // Verify that we can start sending.
     EXPECT_NO_THROW(sender.startSending(io_service));
@@ -336,12 +331,12 @@ TEST(NameChangeUDPSenderBasicTest, basicSendTests) {
         ASSERT_NO_THROW(ncr = NameChangeRequest::fromJSON(valid_msgs[i]));
         EXPECT_NO_THROW(sender.sendRequest(ncr));
         // Verify that the queue count increments in step with each send.
-        EXPECT_EQ(i+1, sender.getQueSize());
+        EXPECT_EQ(i+1, sender.getQueueSize());
     }
 
     // Verify that attempting to send an additional message results in a
     // queue full exception.
-    EXPECT_THROW(sender.sendRequest(ncr), NcrSenderQueFull);
+    EXPECT_THROW(sender.sendRequest(ncr), NcrSenderQueueFull);
 
     // Loop for the number of valid messages and invoke IOService::run_one.
     // This should send exactly one message and the queue count should
@@ -349,37 +344,37 @@ TEST(NameChangeUDPSenderBasicTest, basicSendTests) {
     for (int i = num_msgs; i > 0; i--) {
         io_service.run_one();
         // Verify that the queue count decrements in step with each run.
-        EXPECT_EQ(i-1, sender.getQueSize());
+        EXPECT_EQ(i-1, sender.getQueueSize());
     }
 
     // Verify that the queue is empty.
-    EXPECT_EQ(0, sender.getQueSize());
+    EXPECT_EQ(0, sender.getQueueSize());
 
     // Verify that we can add back to the queue
     EXPECT_NO_THROW(sender.sendRequest(ncr));
-    EXPECT_EQ(1, sender.getQueSize());
+    EXPECT_EQ(1, sender.getQueueSize());
 
     // Verify that we can remove the current entry at the front of the queue.
     EXPECT_NO_THROW(sender.skipNext());
-    EXPECT_EQ(0, sender.getQueSize());
+    EXPECT_EQ(0, sender.getQueueSize());
 
     // Verify that flushing the queue is not allowed in sending state.
-    EXPECT_THROW(sender.flushSendQue(), NcrSenderError);
+    EXPECT_THROW(sender.clearSendQueue(), NcrSenderError);
 
     // Put a message on the queue.
     EXPECT_NO_THROW(sender.sendRequest(ncr));
-    EXPECT_EQ(1, sender.getQueSize());
+    EXPECT_EQ(1, sender.getQueueSize());
 
     // Verify that we can gracefully stop sending.
     EXPECT_NO_THROW(sender.stopSending());
     EXPECT_FALSE(sender.amSending());
 
     // Verify that the queue is preserved after leaving sending state.
-    EXPECT_EQ(1, sender.getQueSize());
+    EXPECT_EQ(1, sender.getQueueSize());
 
     // Verify that flushing the queue works when not sending.
-    EXPECT_NO_THROW(sender.flushSendQue());
-    EXPECT_EQ(0, sender.getQueSize());
+    EXPECT_NO_THROW(sender.clearSendQueue());
+    EXPECT_EQ(0, sender.getQueueSize());
 }
 
 /// @brief Text fixture that allows testing a listener and sender together
@@ -406,12 +401,12 @@ public:
         // Create our listener instance. Note that reuse_address is true.
         listener_.reset(
             new NameChangeUDPListener(addr, LISTENER_PORT, FMT_JSON,
-                                      this, true));
+                                      *this, true));
 
         // Create our sender instance. Note that reuse_address is true.
         sender_.reset(
             new NameChangeUDPSender(addr, SENDER_PORT, addr, LISTENER_PORT,
-                                    FMT_JSON, this, 100, true));
+                                    FMT_JSON, *this, 100, true));
 
         // Set the test timeout to break any running tasks if they hang.
         test_timer_.setup(boost::bind(&NameChangeUDPTest::testTimeoutHandler,
@@ -425,16 +420,16 @@ public:
     }
 
     /// @brief Implements the receive completion handler.
-    virtual void operator ()(NameChangeListener::Result result,
-                             NameChangeRequestPtr ncr) {
+    virtual void operator ()(const NameChangeListener::Result result,
+                             NameChangeRequestPtr& ncr) {
         // save the result and the NCR received.
         recv_result_ = result;
         received_ncrs_.push_back(ncr);
     }
 
     /// @brief Implements the send completion handler.
-    virtual void operator ()(NameChangeSender::Result result,
-                             NameChangeRequestPtr ncr) {
+    virtual void operator ()(const NameChangeSender::Result result,
+                             NameChangeRequestPtr& ncr) {
         // save the result and the NCR sent.
         send_result_ = result;
         sent_ncrs_.push_back(ncr);
@@ -469,16 +464,16 @@ TEST_F (NameChangeUDPTest, roundTripTest) {
         NameChangeRequestPtr ncr;
         ASSERT_NO_THROW(ncr = NameChangeRequest::fromJSON(valid_msgs[i]));
         sender_->sendRequest(ncr);
-        EXPECT_EQ(i+1, sender_->getQueSize());
+        EXPECT_EQ(i+1, sender_->getQueueSize());
     }
 
     // Execute callbacks until we have sent and received all of messages.
-    while (sender_->getQueSize() > 0 || (received_ncrs_.size() < num_msgs)) {
+    while (sender_->getQueueSize() > 0 || (received_ncrs_.size() < num_msgs)) {
         EXPECT_NO_THROW(io_service_.run_one());
     }
 
     // Send queue should be empty.
-    EXPECT_EQ(0, sender_->getQueSize());
+    EXPECT_EQ(0, sender_->getQueueSize());
 
     // We should have the same number of sends and receives as we do messages.
     ASSERT_EQ(num_msgs, sent_ncrs_.size());