On 02:56 pm, [email protected] wrote:
On Wed, Nov 24, 2010 at 3:28 PM, Twisted <[email protected]>
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
_______________________________________________
Twisted-Python mailing list
[email protected]
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
