Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#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: exarkun, jessica.mckellar@…, dustin@… Branch: branches/fd-closer-4881
(diff, github, buildbot, log)
Author: lewq Launchpad Bug:

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)

patch.txt (7.9 KB) - added by lewq 3 years ago.
This patch actually passes the regular tests as well as the new ones
patch.2.txt (8.7 KB) - added by lewq 3 years ago.
With docstrings
close-devnull-after-checking-patch.txt (1.4 KB) - added by lewq 3 years ago.
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).

Download all attachments as: .zip

Change History (33)

comment:1 Changed 3 years ago by lewq

Please review the attached patch, it passes the tests.

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

comment:2 Changed 3 years ago by lewq

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

Changed 3 years ago by lewq

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

Changed 3 years ago by lewq

With docstrings

comment:3 Changed 3 years ago by lewq

Please review patch.2.txt :-)

comment:4 Changed 3 years ago by 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.

comment:5 Changed 3 years ago by lewq

  • Keywords review added

comment:6 Changed 3 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/fd-closer-4881

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

comment:7 Changed 3 years ago by exarkun

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

refs #4881

comment:8 Changed 3 years ago by exarkun

  • Author changed from exarkun to lewq

comment:9 Changed 3 years ago by lvh

  • Keywords review removed

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!

comment:10 Changed 3 years ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:11 Changed 3 years ago by exarkun

(In [31047]) Insert some vertical whitespace

refs #4881

comment:12 Changed 3 years ago by exarkun

(In [31048]) Insert some vertical whitespace

refs #4881

comment:13 Changed 3 years ago by exarkun

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

refs #4881

comment:14 Changed 3 years ago by 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.

comment:15 Changed 3 years ago by 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.

comment:17 Changed 3 years ago by glyph

  • Owner changed from exarkun to glyph
  • Status changed from assigned to new

Reviewing.

comment:18 Changed 3 years ago by 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

comment:19 Changed 3 years ago by glyph

  • Keywords review removed
  • Owner changed from glyph to lewq
  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.

comment:20 Changed 3 years ago by lewq

  • Keywords review added

comment:21 Changed 3 years ago by lvh

  • Owner lewq deleted

comment:22 Changed 3 years ago by lewq

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

comment:23 Changed 3 years ago by jesstess

  • Cc jessica.mckellar@… 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 3 years ago by lewq

  • Resolution set to fixed
  • Status changed from new to 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 3 years ago by 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?

comment:26 Changed 3 years ago by 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?

comment:27 Changed 3 years ago by lewq

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

Changed 3 years ago by 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).

comment:28 Changed 3 years ago by dustin

  • Cc dustin@… added

comment:29 Changed 3 years ago by exarkun

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

comment:30 Changed 3 years ago by exarkun

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

Note: See TracTickets for help on using tickets.