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:23 jinmei]:
 > Okay, then I suggest creating a separate followup ticket so we can
 > eventually fix all known issues and run automatic checker.

 #2043 has been filed for it.

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

 Done.

 > Some other comments follow:
 >
 > '''general'''
 >
 > Maybe we need a changelog?

 How is this:

 {{{
 XXX.    [bug]           muks
         A number of warnings reported by Python about unclosed file and
         socket objects were fixed. Some related code was also made safer.
         (Trac #128, git ...)
 }}}

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

 Looks fine :)

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

 This looks good too.

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

 Updated the variable name.

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

 Done. :)

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