BIND 10 #1828: Python tests print warnings about unclosed file handles / sockets

[BIND 10 Development]

#1828: Python tests print warnings about unclosed file handles / sockets
-------------------------------------+-------------------------------------
                   Reporter:  muks   |                 Owner:  jinmei
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:
                   Priority:         |  Sprint-20120612
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  Unclassified                       |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  4
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 '''general'''

 - is it possible to make the test fail if this warning is triggered?
   since the need for it isn't obvious, it's pretty easy to introduce
   new ones without automatic and demanding check.
 - related to the previous point, I didn't check if this branch fixes
   all current warnings.

 '''bindctl_test.py'''

 Maybe it's a matter of taste, but I guess this may be better:

 {{{#!cpp
         with open(os.devnull, 'w') as f:
             sys.stdout = f
             cmd = bindcmd.BindCmdInterpreter()
             ...
             os.remove(csvfilename)
 }}}

 In the original trac1828 version it's not so obvious to me that why we
 do `sys.stdout.close()`.  And the `with` version is more exception
 safe.

 Also, while it'd probably beyond the scope of this task, it'd be
 better to restore sys.stdout in tearDown(); otherwise if this test
 fails sys.stdout will be kept faked in subsequent tests.

 '''ddns_test'''

 - I suspect the intent of this close may not be obvious and suggest
   adding some comments about why we do it:
 {{{#!python
         if self.ddns_server._listen_socket is not None:
             self.ddns_server._listen_socket.close()
 }}}
 - `TestDDNSServer.test_listen`: it rather seems to be the task of
   `DDNSServer`.  Since it's the one who opens the socket on
   construction, I think it should explicitly close it at least in
   shutdown_cleanup().  Then this test would call this method on
   `ddnss`, rather than trying to close it by itself.

 '''msgq_test'''

 - I think this one should be fixed within msgq: setup_listener()
   should close the socket in the `except` block.
 - I'd simplify infinite_sender() as follows:
 {{{#!python
         with socket.socketpair(socket.AF_UNIX, socket.SOCK_STREAM)[1] as
 write:
             msgq.register_socket(write)
             # Keep sending while it is not closed by the msgq
             try:
                 while True:
                     sender(msgq, write)
             except socket.error:
                 pass
 }}}
 - infinite_sender/send_many: like others, the intent of the close()
   isn't obvious to me.  But in these cases I cannot come up with a
   cleaner alternative.  If this is the least bad one, I'd at least
   clarify the intent in comments.

 '''xfrout_test'''
 - test_guess_remote: `with` should be able to be used in these cases.
 - test_sock_file_in_use: same comment as bindctl_test.py applies.
   besides, I don't even understand why we need to tweak stdout here.
   I guess it was for very older versions that dumped noisy logs
   directly to stdout.  I don't see any noise or other failures even if
   I remove this trick.  I suggest we simply stop tweaking.
 - test_remove_unused_sock_file_dir: likewise.

 '''xfrout.py'''

 - `_guess_remote`: I'd prefer using `with`
 {{{#!python
         if socket.has_ipv6:
             with socket.fromfd(sock_fd, socket.AF_INET6,
 socket.SOCK_STREAM) \
                     as s:
                 peer = s.getpeername()
         else:
             # To make it work even on hosts without IPv6 support
             # (Any idea how to simulate this in test?)
             with socket.fromfd(sock_fd, socket.AF_INET,
 socket.SOCK_STREAM) \
                     as s:
                 peer = s.getpeername()
 }}}
   although admittedly this version doesn't seem to look very simple.
   In any case, 'close()' can be done much earlier than the current
   position.
 - `_sock_file_in_use`: I'd use `with` here (and it would make much
   simpler)

 '''zonemgr_test'''

 - I don't see the need for tweaking stderr here in the first place.
   Also, zonemgr itself has this problem for the socketpair.  What we
   should do is to close them appropriately.

 '''sockcreator_test'''

 - at least this can be revised with `with`:
 {{{#!python
         p1 = socket.fromfd(t2.read_fd(), socket.AF_UNIX,
 socket.SOCK_STREAM)
 }}}

 '''socketsession_test'''

 - shouldn't it better to close self.start_listen in tearDown() (and
   maybe in start_listen() if it can be called multiple times)?
 - check_push_and_pop: at least sock should be able to be managed with
 `with`.
 - test_bad_pop: can use `with`:
 {{{#!python
         with self.accept_forwarder() as accept_sock:
             receiver = SocketSessionReceiver(accept_sock)
             s.close()
             self.assertRaises(SocketSessionError, receiver.pop)
 }}}

-- 
Ticket URL: <http://bind10.isc.org/ticket/1828#comment:20>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development