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

Comment (by jinmei):

 Replying to [comment:22 muks]:

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

 Okay, then I suggest creating a separate followup ticket so we can
 eventually fix all known issues and run automatic checker.

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

 Ah, right, I indeed realized it but I'm not sure why I still made this
 comment.

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

 I guess my concern is subtle, but I still think it's not obvious why
 we need to do the explicit close in general (and someone may think
 it's just redundant and try to "clean them up").  Also, in this
 particular case there's pretty much of code between the points of open
 and close, which would make me wonder what would happen if any of
 the code raises an exception (i.e., if the close() is really
 necessary, it would just look wrong if the code isn't exception safe
 in that sense, and the larger amount of code make it uncertain).

 Maybe one compromise is to add a comment like this:

 {{{#!python
         # Explicitly close temporary socket pair as the Python interpreter
         # expects it.  It may not be 100% exception safe, but since this
 is
         # only for tests we prefer brevity.
         queue.close()
         out.close()
 }}}

 Some other comments follow:

 '''general'''

 Maybe we need a changelog?

 '''ddns.py'''

 - the docstring for shutdown_cleanup is now not 100% correct, so I've
   committed a suggested change (part of which was moved from
   docstring).  Please check.

 '''ddns_test.py'''

 - the intent of calling shutdown_cleanup is probably not so obvious.
   I've committed a suggested comment.  Please check.

 '''xfrout.py'''

 - the revised version of `_guess_remote` looks nice, but I'd rename
   'socktype' to 'sock_family' or something.  The term for sockets
   sounds like SOCK_STREAM/SOCK_DGRAM, etc.

 - `_sock_file_in_use`: the revised version changed the semantics a bit
   in case socket.socket() raises an exception.  I'd suggest revising
   it to:
 {{{#!python
         try:
             with socket.socket(socket.AF_UNIX) as sock:
                 sock.connect(sock_file)
         except socket.error as err:
             return False
         else:
             return True
 }}}

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