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

 Replying to [comment:11 muks]:

 > > My suggestion is to identify the real leak and fix them, and leave
 > > others open, and, ideally, suppress this warning (in general I don't
 > > like to suppress warning, but in this specific case I think it does
 > > more harm than good).
 > >
 > > See also this: http://projects.scipy.org/scipy/ticket/1385
 >
 > This ticket explains the reason why we are seeing these warnings
 (unittests enabling warnings at default level). In that ticket's case,
 they are also silencing the warnings by closing the fds.
 >
 > I don't think we should call `simplefilter()` to suppress all
 `RuntimeWarning` warnings as something important may be missed. It seems
 to be possible to suppress on a line-by-line basis, but it involves
 putting line numbers for suppressions into the code which is, IMO, a worse
 hack than actually closing the objects.

 I agree with this.

 > I have checked every case in the patches in the trac1828 branch now to
 see if there are actual (albeit temporary) leaks to cause exhaustion of
 fds. In each of these cases, the object goes out of scope (exits basic
 block, or the variable is overwritten) and these don't happen in a
 recusive style of looping where the basic block is not exited before other
 fds are alloc'd. So the patches help in suppressing the `RuntimeWarning`s
 alone, but the `close()` on the underlying fds should happen anyway
 without the patches.

 I'm afraid I don't understand this...

 - In "the object goes out of scope" case, the underlying FD should be
   immediately closed (with the warning), so it basically shouldn't
   cause the descriptor exhaustion (as long as the scope is
   sufficiently small), right?
 - "these don't happen in a recu(r)sive style..." is not clear to me.
   Are these the points that actually cause the FD exhaustion?

 But, in any case,

 > Also, the warnings are not caused by syntax checks but happen during
 runtime in the .c module when the last dereference causes a check on the
 underlying fd to see if it's a valid one (!= -1 in socketmodule.c, or > 0
 in fileio.c). I don't know why the Python devs such checks, but they
 actually expect the explicit `.close()` to be performed then.

 hmm, I see, you're probably right on this.  So it appears that doing
 explicit close() is the expected practice in Python, even if the file
 object will be GC'ed eventually and the file will be closed at that
 point.  In that case I don't object to following that convention.

 But since it's not so obvious why we need to do the seemingly
 redundant close(), I'd suggest updating the coding guideline page
 http://bind10.isc.org/wiki/CodingGuidelines#PythonStyle
 to explain this issue and what we generally should do in our code.  I
 also suggest raising this at the bind10-dev list so other developers
 can recognize it.

 Regarding the branch itself...

 - (repeating my original comment) I'd suggest using 'file' or
   at least something different from 'fd' to represent file object.
 - it would probably be even better to use the `with` idiom so the code
   is more exception safe in terms of the timing of close().  That is,
   instead of
 {{{#!python
     file = open(self.pid_file, "w")
     file.write('dummy data\n')
     file.close()
 }}}
   do this:
 {{{#!python
     # or in this simple case it could be a one-liner.
     with open(self.pid_file, "w") as file:
         file.write('dummy data\n')
 }}}

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