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

 * owner:  muks => jinmei


Comment:

 Replying to [comment:9 jinmei]:
 > I've not fully reviewed the code yet, but have some initial comments:
 >
 > First off, do we really need things like this?
 > {{{#!diff
 > -        open(self.pid_file, "w").write('dummy data\n')
 > +        fd = open(self.pid_file, "w")
 > +        fd.write('dummy data\n')
 > +        fd.close()
 > }}}
 > As Shane mentioned in #845, this is very counter intuitive, and if we
 > really need to explicitly close opened files this way, it seems to me
 > something is very wrong.  At the very least I don't think we can
 > reliably keep this convention in code we'll develop in future (except
 > by seeing the warnings and modifying the code to silence them).

 By counter-intuitive, I gather you mean having to `close()` the
 file/socket object manually, and not the verbosity of the code itself.

 > I've written the following simple test code:
 >
 > {{{#!python
 > while True:
 >     open('f.py', 'r')
 > }}}

 Nod. It does destroy the object (and `close()` the underlying fd) at the
 block exit where it goes out of scope.

 The interpreter also destroys immediately an old object when a variable is
 overwritten, though it may not be right to assume that this non-lazy
 behavior (it seems to be refcounted) will exist in future versions or
 other interpreters.

 {{{
 a = Something()
 a = Something() # first Something is destroyed immediately after
                 # the second Something's construction, during assignment
 }}}

 > So, I suspect there are only a small number of real leak (or very
 delayed
 > close), and the others are false alarm.  I also suspect this warning
 > is quite naive and just performs simple syntax level checks (e.g.,
 > whether explicit close() follows an open()).  While seeing too noisy
 > warning is not a good thing, I don't like to tweak our code in a
 > counter intuitive manner just to silence the warning.
 >
 > 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 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.

 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.

 So in light of this, shall we keep this branch to suppress the warnings,
 or abandon it?

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