Browse Source

[3970] Propagate errors emitted when closing the WatchSocket.

Marcin Siodelski 9 years ago
parent
commit
c3a1e5668e

+ 4 - 10
src/lib/dhcp_ddns/dhcp_ddns_messages.mes

@@ -89,14 +89,8 @@ 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.
 
-% DHCP_DDNS_WATCH_SINK_CLOSE_ERROR Sink-side watch socket failed to close: %1
+% DHCP_DDNS_UDP_SENDER_WATCH_SOCKET_CLOSE_ERROR watch socket failed to close: %1
 This is an error message that indicates the application was unable to close
-the inbound side of a NCR sender's watch socket.  While technically possible
-this error is highly unlikely to occur and should not impair the application's
-ability to process requests.
-
-% DHCP_DDNS_WATCH_SOURCE_CLOSE_ERROR Source-side watch socket failed to close: %1
-This is an error message that indicates the application was unable to close
-the outbound side of a NCR sender's watch socket.  While technically possible
-this error is highly unlikely to occur and should not impair the application's
-ability to process requests.
+the inbound or outbound side of a NCR sender's watch socket. While technically
+possible the error is highly unlikely to occur and should not impair the
+application's ability to process requests.

+ 13 - 1
src/lib/dhcp_ddns/ncr_udp.cc

@@ -260,6 +260,7 @@ NameChangeUDPSender::open(isc::asiolink::IOService& io_service) {
 
     send_callback_->setDataSource(server_endpoint_);
 
+    closeWatchSocket();
     watch_socket_.reset(new util::WatchSocket());
 }
 
@@ -288,6 +289,7 @@ NameChangeUDPSender::close() {
 
     socket_.reset();
 
+    closeWatchSocket();
     watch_socket_.reset();
 }
 
@@ -372,7 +374,17 @@ NameChangeUDPSender::ioReady() {
     return (false);
 }
 
-
+void
+NameChangeUDPSender::closeWatchSocket() {
+    if (watch_socket_) {
+        std::string error_string;
+        watch_socket_->closeSocket(error_string);
+        if (!error_string.empty()) {
+            LOG_ERROR(dhcp_ddns_logger, DHCP_DDNS_UDP_SENDER_WATCH_SOCKET_CLOSE_ERROR)
+                .arg(error_string);
+        }
+    }
+}
 
 }; // end of isc::dhcp_ddns namespace
 }; // end of isc namespace

+ 7 - 0
src/lib/dhcp_ddns/ncr_udp.h

@@ -548,6 +548,13 @@ public:
     virtual bool ioReady();
 
 private:
+
+    /// @brief Closes watch socket if the socket is open.
+    ///
+    /// This method closes watch socket if its instance exists and if the
+    /// socket is open. An error message is logged when this operation fails.
+    void closeWatchSocket();
+
     /// @brief IP address from which to send.
     isc::asiolink::IOAddress ip_address_;
 

+ 23 - 0
src/lib/util/tests/watch_socket_unittests.cc

@@ -231,4 +231,27 @@ TEST(WatchSocketTest, badReadOnClear) {
     ASSERT_THROW(watch->markReady(), WatchSocketError);
 }
 
+/// @brief Checks if the socket can be explicitly closed.
+TEST(WatchSocketTest, explicitClose) {
+    WatchSocketPtr watch;
+
+    // Create new instance of the socket.
+    ASSERT_NO_THROW(watch.reset(new WatchSocket()));
+    ASSERT_TRUE(watch);
+
+    // Make sure it has been opened by checking that its descriptor
+    // is valid.
+    EXPECT_NE(watch->getSelectFd(), WatchSocket::SOCKET_NOT_VALID);
+
+    // Close the socket.
+    std::string error_string;
+    ASSERT_TRUE(watch->closeSocket(error_string));
+
+    // Make sure that the descriptor is now invalid which indicates
+    // that the socket has been closed.
+    EXPECT_EQ(WatchSocket::SOCKET_NOT_VALID, watch->getSelectFd());
+    // No errors should be reported.
+    EXPECT_TRUE(error_string.empty());
+}
+
 } // end of anonymous namespace

+ 20 - 8
src/lib/util/watch_socket.cc

@@ -116,17 +116,19 @@ WatchSocket::clearReady() {
     }
 }
 
-void
-WatchSocket::closeSocket() {
+bool
+WatchSocket::closeSocket(std::string& error_string) {
+    // Clear error string.
+    error_string.clear();
+
     // Close the pipe fds.  Technically a close can fail (hugely unlikely)
     // but there's no recovery for it either.  If one does fail we log it
     // and go on. Plus this is called by the destructor and no one likes
     // destructors that throw.
     if (source_ != SOCKET_NOT_VALID) {
         if (close(source_)) {
-/*            const char* errstr = strerror(errno);
-            LOG_ERROR(dhcp_ddns_logger, DHCP_DDNS_WATCH_SOURCE_CLOSE_ERROR)
-                      .arg(errstr); */
+            // An error occured.
+            error_string = strerror(errno);
         }
 
         source_ = SOCKET_NOT_VALID;
@@ -134,13 +136,23 @@ WatchSocket::closeSocket() {
 
     if (sink_ != SOCKET_NOT_VALID) {
         if (close(sink_)) {
-/*            const char* errstr = strerror(errno);
-            LOG_ERROR(dhcp_ddns_logger, DHCP_DDNS_WATCH_SINK_CLOSE_ERROR)
-                      .arg(errstr); */
+            // An error occured.
+            if (error_string.empty()) {
+                error_string = strerror(errno);
+            }
         }
 
         sink_ = SOCKET_NOT_VALID;
     }
+
+    // If any errors have been reported, return false.
+    return (error_string.empty() ? true : false);
+}
+
+void
+WatchSocket::closeSocket() {
+    std::string error_string;
+    closeSocket(error_string);
 }
 
 int

+ 14 - 2
src/lib/util/watch_socket.h

@@ -114,11 +114,23 @@ public:
     /// pipe.
     int getSelectFd();
 
+    /// @brief Closes the descriptors associated with the socket.
+    ///
+    /// This method is used to close the socket and capture errors that
+    /// may occur during this operation.
+    ///
+    /// @param [out] error_string Holds the error string if closing
+    /// the socket failed. It will hold empty string otherwise.
+    ///
+    /// @return true if the operation was successful, false otherwise.
+    bool closeSocket(std::string& error_string);
+
 private:
+
     /// @brief Closes the descriptors associated with the socket.
     ///
-    /// Used internally in the destructor and if an error occurs marking or
-    /// clearing the socket.
+    /// This method is called by the class destructor and it ignores
+    /// any errors that may occur while closing the sockets.
     void closeSocket();
 
     /// @brief The end of the pipe to which the marker is written