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-20120619
  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      |
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => jinmei


Comment:

 Replying to [comment:20 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.

 It is possible to assert if a warning is generated by using
 `warnings.catch_warnings()', but as you note, not all warnings are fixed
 by this branch as the reason for some are not obvious.

 > '''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.

 Done :)

 > '''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()
 > }}}

 Comment has been added.

 > - `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.

 Done.

 > '''msgq_test'''
 >
 > - I think this one should be fixed within msgq: setup_listener()
 >   should close the socket in the `except` block.

 Done.

 > - 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
 > }}}

 This `with` statement leaves pair[0] open and generates a warning.

 > - 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.

 Same condition as above, I've not used `with` for these. The `.close()`
 itself should not need explanation, no? We are closing when done with the
 object.

 > '''xfrout_test'''
 > - test_guess_remote: `with` should be able to be used in these cases.

 Uses `with` now.

 > - 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.

 Done.

 > - test_remove_unused_sock_file_dir: likewise.

 Done.

 >
 > '''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.

 I've made it look like this now:

 {{{
         if socket.has_ipv6:
             socktype = socket.AF_INET6
         else:
             # To make it work even on hosts without IPv6 support
             # (Any idea how to simulate this in test?)
             socktype = socket.AF_INET

         with socket.fromfd(sock_fd, socktype, socket.SOCK_STREAM) as sock:
             peer = sock.getpeername()
 }}}

 > - `_sock_file_in_use`: I'd use `with` here (and it would make much
 >   simpler)

 Done.

 > '''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.

 Done.

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

 Done.

 >
 > '''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)?

 This has been fixed now.

 > - check_push_and_pop: at least sock should be able to be managed with
 `with`.

 Done.

 > - 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)
 > }}}

 Done.

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