[2617] make sure FD handling is performed only when it's still valid.

This eliminates the possibility of hitting MSGQ_READ_UNKNOWN_FD, and, in fact,
we can even deprecate this message.   And then, process_socket() becomes
a 2-line function called only by _process_fd() and can be integrated into
the latter.  so it's done, too.

src/bin/msgq/msgq.py.in

@@ -319,14 +319,6 @@ class MsgQ:
         else:
             self.add_kqueue_socket(newsocket)
 
-    def process_socket(self, fd):
-        """Process a read on a socket."""
-        if not fd in self.sockets:
-            logger.error(MSGQ_READ_UNKNOWN_FD, fd)
-            return
-        sock = self.sockets[fd]
-        self.process_packet(fd, sock)
-
     def kill_socket(self, fd, sock):
         """Fully close down the socket."""
         if self.poller:
@@ -639,11 +631,14 @@ class MsgQ:
         from tests.
 
         '''
-        if writable:
+        # We need to check if FD is still in the sockets dict, because
+        # it's possible that the socket has been "killed" while processing
+        # other FDs; it's even possible it's killed within this method.
+        if writable and fd in self.sockets:
             self.__process_write(fd)
-        if readable:
-            self.process_socket(fd)
-        if closed:
+        if readable and fd in self.sockets:
+            self.process_packet(fd, self.sockets[fd])
+        if closed and fd in self.sockets:
             self.kill_socket(fd, self.sockets[fd])
 
     def stop(self):

src/bin/msgq/msgq_messages.mes

@@ -68,11 +68,6 @@ happen and it is either a programmer error or OS bug. The event is ignored. The
 number noted as the event is the raw encoded value, which might be useful to
 the authors when figuring the problem out.
 
-% MSGQ_READ_UNKNOWN_FD Got read on strange socket %1
-The OS reported a file descriptor is ready to read. But the daemon doesn't know
-the mentioned file descriptor, which is either a programmer error or OS bug.
-The read event is ignored.
-
 % MSGQ_RECV_ERR Error reading from socket %1: %2
 There was a low-level error when reading from a socket. The error is logged and
 the corresponding socket is dropped.

src/bin/msgq/tests/msgq_test.py

@@ -588,22 +588,26 @@ class SocketTests(unittest.TestCase):
             self.error_called = 0
             self.warn_called = 0
             self.orig_logger = logger
-        def error(self, id, fd_arg, sock_arg):
+        def error(self, *args):
             self.error_called += 1
-            self.orig_logger.error(id, fd_arg, sock_arg)
-        def warn(self, id, fd_arg):
+            self.orig_logger.error(*args)
+        def warn(self, *args):
             self.warn_called += 1
-            self.orig_logger.warn(id, fd_arg)
+            self.orig_logger.warn(*args)
 
     def mock_kill_socket(self, fileno, sock):
         '''A replacement of MsgQ.kill_socket method for inspection.'''
         self.__killed_socket = (fileno, sock)
+        if fileno in self.__msgq.sockets:
+            del self.__msgq.sockets[fileno]
 
     def setUp(self):
         self.__msgq = MsgQ()
         self.__msgq.kill_socket = self.mock_kill_socket
         self.__sock = self.MockSocket()
         self.__data = b'dummy'
+        self.__msgq.sockets[42] = self.__sock
+        self.__msgq.sendbuffs[42] = (None, b'testdata')
         self.__sock_error = socket.error()
         self.__killed_socket = None
         self.__logger = self.LoggerWrapper(msgq.logger)
@@ -657,6 +661,32 @@ class SocketTests(unittest.TestCase):
             self.assertEqual(expected_errors, self.__logger.error_called)
             self.assertEqual(expected_warns, self.__logger.warn_called)
 
+    def test_process_fd_read_after_bad_write(self):
+        '''Check the specific case of write fail followed by read attempt.
+
+        The write failure results in kill_socket, then read shouldn't tried.
+
+        '''
+        self.__sock_error.errno = errno.EPIPE
+        self.__sock.ex_on_send = self.__sock_error
+        self.__msgq.process_socket = None # if called, trigger an exception
+        self.__msgq._process_fd(42, True, True, False) # shouldn't crash
+
+        # check the socket is deleted from the fileno=>sock dictionary
+        self.assertEqual({}, self.__msgq.sockets)
+
+    def test_process_fd_close_after_bad_write(self):
+        '''Similar to the previous, but for checking dup'ed kill attempt'''
+        self.__sock_error.errno = errno.EPIPE
+        self.__sock.ex_on_send = self.__sock_error
+        self.__msgq._process_fd(42, True, False, True) # shouldn't crash
+        self.assertEqual({}, self.__msgq.sockets)
+
+    def test_process_fd_writer_after_close(self):
+        '''Emulate a "writable" socket has been already closed and killed.'''
+        # This just shouldn't crash
+        self.__msgq._process_fd(4200, True, False, False)
+
 if __name__ == '__main__':
     isc.log.resetUnitTestRootLogger()
     unittest.main()