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

[exarkun at twistedmatrix.com]

On 02:56 pm, eric.faurot at gmail.com wrote:
>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.

Maybe so.  Fortunately the problem really seems only to be with the 
_test_, right?  On OpenBSD the fallback case will really be used, and 
the file descriptors we want to close will be closed?
>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.

Right, and this is just what should happen on OpenBSD now.
>
>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...

Hopefully these are already marked as CLOEXEC, or lots of people would 
already be having lots of problems with them.  Despite that, they should 
be closed by `reactor.spawnProcess` (because that doesn't hurt and it's 
simpler than checking for CLOEXEC).  Then the only problem is that they 
are recreated in the child process and mess up the test's assumptions.

Perhaps another approach to testing this function would be to create a 
file descriptor and assert that:

  - That file descriptor is not open in the child process
  - stdin, stdout, and sterr are open in the child process

Only, without some care, one of the pthread pipe fds might end up 
looking like the probe descriptor, so maybe that can't really work.
>> 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()

closefrom() doesn't appear to help.  Descriptors intended to be kept 
open may be mixed together with descriptors which must be closed.  This 
is because of the `childFDs` parameter to `reactor.spawnProcess`, which 
allows arbitrary file descriptors to be passed to the child.

Jean-Paul