twisted.internet.process._listOpenFDs fails on FreeBSD because fdescfs isn't mounted by default

[lewq]

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.

[lewq]

Please review the attached patch, it passes the tests.

Still needs doc-strings on the classes and functions though.

[lewq]

(I.e. please sanity check my approach. If it's right, I'll tidy it up and submit it for full review).

[lewq]

This patch actually passes the regular tests as well as the new ones

[lewq]

With docstrings

[lewq]

Please review patch.2.txt :-)

[lewq]

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.

[Jean-Paul Calderone]

(In [30903]) Branching to 'fd-closer-4881'

[Jean-Paul Calderone]

(In [30904]) Apply patch.2.txt

refs #4881

[lvh]

Hi! Thanks for your patch :-)

1. Twisted coding standard: should be 3 empty lines between the functions.
2. "Instanciate a (testable) _FDDetector which actually does the work." Instanciate -> instantiate
3. 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.
4. In _checkDevFDSanity, why is the opened /dev/null assigned to a name? It doesn't seem to be used.
5. FDDetectorTest.save_resource_module also has a bare except.

Everything else looks great :) Thanks again!

[Jean-Paul Calderone]

(In [31047]) Insert some vertical whitespace

refs #4881

[Jean-Paul Calderone]

(In [31048]) Insert some vertical whitespace

refs #4881

[Jean-Paul Calderone]

(In [31055]) Document FDDetectorTest attributes, rename some methods to conform to convention, simplify tests with addCleanup, fix some extra indentation

refs #4881

[lewq]

# 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.

[lewq]

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.

[lewq]

Passing tests, see ​http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/fd-closer-4881

[Glyph]

Reviewing.

[Glyph]

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

[Glyph]

1. FDDetectorTest is missing a couple of docstrings. getpid, listdirand setUp.
2. FakeResourceModule is missing a docstring for getrlimit.
3. Is that TODO still valid? If it is, file a ticket and link to it. Otherwise just delete it.
4. _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.
5. 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 :).
6. _FDDetector._listOpenFDs needs a docstring.
7. Some @ivars for listdir, getpid, openfile on that class would be useful as well.
8. 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.
9. _defFDImplementation could use a reference of some kind; if you can find a document to link to that would be great.
10. 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.

[lewq]

​http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/fd-closer-4881 appears to be working nicely now, please review :-)

[lewq]

I've addressed all of the issues in comment 19, by the way.

[jesstess]

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!

[lewq]

(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.

[Glyph]

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?

[lewq]

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?

[lewq]

(After running _listOpenFDs() at least once so that it does the detection)

[lewq]

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).

[Jean-Paul Calderone]

I filed #5008 for the remaining /dev/null issue.

[Jean-Paul Calderone]

This was sort of a duplicate of #4747, but that ticket also talks about one more shortcoming of the unit tests.