#4881 defect closed fixed (fixed)
twisted.internet.process._listOpenFDs fails on FreeBSD because fdescfs isn't mounted by default
Reported by: | lewq | Owned by: | lewq |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | core | Keywords: | |
Cc: | Jean-Paul Calderone, jesstess, Dustin J. Mitchell | Branch: |
branches/fd-closer-4881
branch-diff, diff-cov, branch-cov, buildbot |
Author: | lewq |
Description
The solution to this is to dynamically detect whether /dev/fd/* is reliable or not, by opening some files and checking.
This breaks 10.2 on FreeBSD.
Attachments (3)
Change History (33)
comment:1 Changed 11 years ago by
comment:2 Changed 11 years ago by
(I.e. please sanity check my approach. If it's right, I'll tidy it up and submit it for full review).
Changed 11 years ago by
This patch actually passes the regular tests as well as the new ones
comment:4 Changed 11 years ago by
Explanation of this issue and a workaround:
Twisted 10.2 added an optimisation which assumes that /dev/fd/*
contains the current process's open file descriptors. On FreeBSD this is only true if you run mount -t fdescfs null /dev/fd
.
If you are running Twisted 10.2 on FreeBSD you should add that to your fstab so that it survives a reboot.
comment:5 Changed 11 years ago by
Keywords: | review added |
---|
comment:6 Changed 11 years ago by
Author: | → exarkun |
---|---|
Branch: | → branches/fd-closer-4881 |
(In [30903]) Branching to 'fd-closer-4881'
comment:8 Changed 11 years ago by
Author: | exarkun → lewq |
---|
comment:9 Changed 11 years ago by
Keywords: | review removed |
---|
Hi! Thanks for your patch :-)
- Twisted coding standard: should be 3 empty lines between the functions.
- "Instanciate a (testable) _FDDetector which actually does the work." Instanciate -> instantiate
- There's already some bare excepts there that shouldn't have been, but I think your patch adds a new one. It should probably catch a specific exception. I'm thinking OSError.
- In
_checkDevFDSanity
, why is the opened /dev/null assigned to a name? It doesn't seem to be used. - FDDetectorTest.save_resource_module also has a bare except.
Everything else looks great :) Thanks again!
comment:10 Changed 11 years ago by
Owner: | set to Jean-Paul Calderone |
---|---|
Status: | new → assigned |
comment:13 Changed 11 years ago by
comment:14 Changed 11 years ago by
# In _checkDevFDSanity, why is the opened /dev/null assigned to a name? It doesn't seem to be used.
fp = self.openfile("/dev/null", "r") end = self.listdir("/dev/fd")
If the open file were not assigned, it would have zero references and then might it get GC'd before listdir is run?
JP, your concern that _listOpenFDs doesn't actually get set is because:
def _setFDImplementation(): [...] _listOpenFDs = detector._getImplementation()
... only sets _listOpenFDs in the function-local scope. The addition of the 'global' keyword at the top of the function should fix this, right? I'll test it by checking the identity of the function in a unit test.
comment:15 Changed 11 years ago by
NB: If fdescfs on FreeBSD gets (un)mounted during the running of a Twisted program, it would be nice for us to be checking os.path.ismount on /dev/fd. This is a future correctness improvement.
comment:16 Changed 11 years ago by
Keywords: | review added |
---|
comment:17 Changed 11 years ago by
Owner: | changed from Jean-Paul Calderone to Glyph |
---|---|
Status: | assigned → new |
Reviewing.
comment:18 Changed 11 years ago by
One thing I can see that needs some fixing is that twisted.internet.process is now imported by test_process. The things that test_process tests are implemented by modules other than twisted.internet.process on Windows, which is why it pokes the reactor. See this failure - http://buildbot.twistedmatrix.com/builders/winxp32-py2.5/builds/672/steps/iocp/logs/problems
[ERROR] Traceback (most recent call last): File "C:\twistedbot\winxp32-py2.5-select\Twisted\twisted\trial\runner.py", line 575, in loadPackage module = modinfo.load() File "C:\twistedbot\winxp32-py2.5-select\Twisted\twisted\python\modules.py", line 383, in load return self.pathEntry.pythonPath.moduleLoader(self.name) File "C:\twistedbot\winxp32-py2.5-select\Twisted\twisted\python\reflect.py", line 464, in namedAny topLevelPackage = _importAndCheckStack(trialname) File "C:\twistedbot\winxp32-py2.5-select\Twisted\twisted\python\reflect.py", line 400, in _importAndCheckStack return __import__(importName) File "C:\twistedbot\winxp32-py2.5-select\Twisted\twisted\internet\test\test_process.py", line 25, in <module> from twisted.internet import process File "C:\twistedbot\winxp32-py2.5-select\Twisted\twisted\internet\process.py", line 103, in <module> detectLinuxBrokenPipeBehavior() File "C:\twistedbot\winxp32-py2.5-select\Twisted\twisted\internet\process.py", line 93, in detectLinuxBrokenPipeBehavior reads, writes, exes = select.select([w], [], [], 0) select.error: (10038, 'An operation was attempted on something that is not a socket') twisted.internet.test.test_process
comment:19 Changed 11 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Glyph to lewq |
FDDetectorTest
is missing a couple of docstrings.getpid
,listdir
andsetUp
.FakeResourceModule
is missing a docstring forgetrlimit
.- Is that TODO still valid? If it is, file a ticket and link to it. Otherwise just delete it.
_resourceFDImplementation
should note that on OS X, we should be using one of the other implementations, but if we are on some platform which returns a similar value, we should not actually try to close all those FDs.- You're wrapping docstring lines a bit early in many places. Since I know you're a vim user: Vjgqq on your docstring paragraphs to rewrap them, with the appropriate number of 'j's :).
_FDDetector._listOpenFDs
needs a docstring.- Some
@ivar
s for listdir, getpid, openfile on that class would be useful as well. FDDetector
's docstring should be modified so it doesn't refer to "the above implementations", since the implementations are actually below, and members of the class. Write a docstring that explains, from scratch, why we need such a thing. It might make sense to change the name to_OpenFDDetector
, too; I leave that to you, though._defFDImplementation
could use a reference of some kind; if you can find a document to link to that would be great.- You need to write a NEWS file.
Aside from the functional issue raised in the previous comment, this actually looks pretty good; these are mostly minor maintenance documentation issues. I'd like to re-review just to get a second set of eyes on those test results, but that's all; we should be able to merge it quickly thereafter.
comment:20 Changed 11 years ago by
Keywords: | review added |
---|
http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/fd-closer-4881 appears to be working nicely now, please review :-)
comment:21 Changed 11 years ago by
Owner: | lewq deleted |
---|
comment:23 Changed 11 years ago by
Cc: | jesstess added |
---|---|
Keywords: | review removed |
Owner: | set to lewq |
Thanks for sticking with this, lewq!
+ See: http://www.freebsd.org/cgi/man.cgi?fdescfs
Use @see.
Other than that, I agree that the builds look fine and the FreeBSD build looks better.
Please merge!
comment:24 Changed 11 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(In [31322]) Merge fd-closer-4881: Fix spawnProcess on FreeBSD.
Author: lewq Reviewer: exarkun, glyph, jesstess, lvh Fixes: #4881
twisted.internet.process now detects the most appropriate mechanism to use for detecting the open file descriptors on a system, getting Twisted working on FreeBSD even when fdescfs is not mounted.
comment:25 Changed 11 years ago by
So, I'm not reopening this, because it's not an issue on the buildbot, but at least on my local mac, I see this:
[ERROR] Traceback (most recent call last): File "/Domicile/glyph/Downloads/Twisted-11.0.0pre1/twisted/internet/_baseprocess.py", line 60, in maybeCallProcessEnded proto.processEnded(Failure(reason)) File "/Domicile/glyph/Downloads/Twisted-11.0.0pre1/twisted/internet/test/test_process.py", line 289, in processEnded checkOutput("".join(self.output)) File "/Domicile/glyph/Downloads/Twisted-11.0.0pre1/twisted/internet/test/test_process.py", line 276, in checkOutput self.assertEquals('[0, 1, 2, 3]', output) twisted.trial.unittest.FailTest: not equal: a = '[0, 1, 2, 3]' b = '[0, 1, 2, 3, 6, 7, 8]' twisted.internet.test.test_process.PTYProcessTestsBuilder_CFReactor.test_openFileDescriptors twisted.internet.test.test_process.PTYProcessTestsBuilder_SelectReactor.test_openFileDescriptors twisted.internet.test.test_process.ProcessTestsBuilder_CFReactor.test_openFileDescriptors twisted.internet.test.test_process.ProcessTestsBuilder_SelectReactor.test_openFileDescriptors
Any idea what that might be about?
comment:26 Changed 11 years ago by
Not sure, what's the configuration difference between buildbot and your Mac? Is it the same version of the OS? Can you check whether process.detector._listOpenFDs.func_name is the same on the BuildBot vs your Mac?
comment:27 Changed 11 years ago by
(After running _listOpenFDs() at least once so that it does the detection)
Changed 11 years ago by
Attachment: | close-devnull-after-checking-patch.txt added |
---|
Attaching a patch which forces the explicit closing of the file pointer to /dev/null in _checkDevFDSanity in case that's hanging around and polluting the list of file descriptors somehow (although it shouldn't cause three extra FDs).
comment:28 Changed 11 years ago by
Cc: | Dustin J. Mitchell added |
---|
comment:30 Changed 11 years ago by
This was sort of a duplicate of #4747, but that ticket also talks about one more shortcoming of the unit tests.
Please review the attached patch, it passes the tests.
Still needs doc-strings on the classes and functions though.