[Twisted-Python] [Twisted] #4747: tw.inet.test.ProcessTestsBuilder.test_openFileDescriptors is broken in many ways

[Eric Faurot]

On Wed, Nov 24, 2010 at 3:28 PM, Twisted <trac at twistedmatrix.com> wrote:
> #4747: tw.inet.test.ProcessTestsBuilder.test_openFileDescriptors is broken in many
> ways
> ----------------------------+-----------------------------------------------
>     Reporter:  soyt        |           Owner:  soyt
>         Type:  regression  |          Status:  new
>     Priority:  normal      |       Milestone:  Twisted 10.2
>    Component:  core        |        Keywords:
>       Branch:              |   Branch_author:
> Launchpad_bug:              |
> ----------------------------+-----------------------------------------------
> Changes (by exarkun):
>
>  * owner:  glyph => soyt
>  * keywords:  review =>
>
>
> Comment:
>
>  Thanks very much for the patch.  We don't have any OpenBSD machines for
>  testing, or I suppose we would have noticed this sooner. :)
>
>  To answer the question of what functionality it provides that's worth
>  testing...  `_listOpenFDs` is used by the `reactor.spawnProcess` support
>  to find out which file descriptors need to be closed so that they aren't
>  inherited by the child process.  So, it must work right otherwise the
>  child may inherit file descriptors you did not want it to inherit.  This
>  is an optimization over trying to close (roughly) every integer in the
>  range (3, RLIMIT_NFILES), where RLIMIT_NFILES is often around 1024.

So in my opinion, the function should really return the list of actually
opened files in all cases, and not fallback to a list of possible
list, otherwise
the test case makes no sense. Of course this means trying all fds as last resort
on platforms that don't provide a simpler way to do it.  Doing it there or
doing it by ignoring EBADF on close() later would be the same, as far as
"suboptimization" is concerned.

Another (nicer?) possibility to achieve the "close non standard fds" would be
to use closefrom().

>  I don't think the patch is quite correct, since ''at most'' the one extra
>  `os.listdir` file descriptor should be in the result set.  Anything more
>  than that indicates the desired functionality isn't being provided.  The
>  more correct test assertion would be that the output is ''either''
>  range(3) or range(4).  However, I'm not sure what you mean by the "uses a
>  pair of interconnected pipes" comment.  Uses it for what?  To implement
>  `os.listdir`?

No, it's much worse :)
While trying to fix the issue, I was surpised to notice that python had 5 opened
fds by default here... After digging a bit further, I found out it is
actually due to
python being linked with libpthread, which installs a pair of pipe at init...

>  It would be nice if we didn't have to scan and close all of
>  range(RLIMIT_NFILES) on OpenBSD though.  Do you know if there is any way
>  to get this information on that platform?

For the general case no, but for the intended purpose I'd say use closefrom()

Eric.