improve the tw.internet.process._FDDetector_getImplementation() fallback chain

[soyt]

When _checkDevFDSanity() fails, _resourceFDImplementation() should be tried before _fallbackImplementation(), because it is "more correct". The attached patch does that and fixes the regress test accordingly.

[exarkun]

(In [31759]) Branching to 'listopenfds-resource-fallback-5052'

[exarkun]

(In [31760]) Apply soyt's fallback.diff

refs #5052

[exarkun]

1. The docstrings of _getImplementation and _listOpenFDs are a little confusing (they were before this patch too - not your fault). Rather than just shuffle the confusion around, I think we should clean them up.
2. The change to the test to assert that the result is either one of two possible results is wrong. It should always be the resource module implementation

[exarkun]

As well as addressing the review comments, I went a bit further:

1. I moved the tests into test_posixprocess.py, since they are whitebox tests, not blackbox IReactorProcess tests.
2. I simplified the implementation of _getImplementation by generalizing it to work on an arbitrary list of possible implementations.
3. This led to a simplification of the tests, since it's no longer necessary to test a large number of combinations of possible system state to exercise all code paths. The only code paths are "if this one worked, return it; if not, try the next one".

[exarkun]

 Latest build results, btw.

[glyph]

Reviewing.

[glyph]

LGTM. Land.

[exarkun]

As I was writing the commit message, I realized the issue this ticket described isn't really resolved. The implementation selection logic cannot choose _resourceFDImplementation because it always returns the same result. The only implementation option which will be used if it always returns the same result is the final one, which is _fallbackFDImplementation in the branch.

[retenodus]

Why not just do :

for impl in self._implementations:
    try:
        impl()
    except:
        continue

?

I don't see what is the benefits of before and after in the current process._getImplementation()

[exarkun]

(In [32349]) Combine _resourceFDImplementation and _fallbackFDImplementation so that the resource module will be used if it is available.

refs #5052

[exarkun]

(In [32351]) Branching to 'listopenfds-resource-fallback-5052-2'

[retenodus]

There is an error in the docstring : you changed the default value returned by _fallbackFDImplementation from 256 to 1024 but you let 256 in the docstring.

[exarkun]

(In [32353]) Don't try to duplicate the implementation default in the docstring, just give a general idea of what's going on.

refs #5052

[exarkun]

Thanks Retenodus. :)

I guess the branch is ready for another review. The resource module is now used by the fallback implementation, so if it is importable it will be used.

 Build results.

[jesstess]

soyt, thanks for the report and initial patch. exarkun, this looks great and much cleaner than the implementation on master.

I have one question: if I'm on a machine where _procFDImplementation and _devFDImplementation don't work and I either don't have the resource module or am on a Mac so resource.getrlimit gets rounded down to 1024, _getImplementation will return nothing because of the before != after check. e.g. (on a Mac):

>>> from twisted.internet.process import _FDDetector
>>> f1 = _FDDetector()
>>> f1._implementations = [f1._fallbackFDImplementation]
>>> print f1._getImplementation()
None
>>>

Should _getImplementation be returning _fallbackFDImplementation here, or have I managed to fabricate a situation that can't happen?

[glyph]

Replying to jesstess:

Should _getImplementation be returning _fallbackFDImplementation here, or have I managed to fabricate a situation that can't happen?

Looking at the tests here, you've spotted a pretty serious bug. The direct tests still don't appear to cover the case where _fallbackFDImplementation should be returned - this is the same bug that exarkun mentioned way back in comment #9. It will definitely want to be returned on some platforms, although mainly those we don't support yet. I think that this is the problem that has nothing to do with being on a mac, since the resource-based implementation will always return the same value too (just based on a more ostensibly 'correct' maximum), and therefore that loop will still exit returning None.

Also, why's the resource module imported in a try/except? This implementation is only used on unix-y platforms, and resource is always available on unix, no?  http://docs.python.org/library/resource.html seems to imply that it will be.

I looked at the  OpenBSD buildbot results with trunk, and  then ran a build on this branch, but it hasn't completed yet as of this writing, and it appears wedged. Maybe when the build ultimately times out we'll get some output flushed indicating that the bug that's causing it to hang is relevant to this branch.

[exarkun]

(In [32439]) Return the last implementation if none of the implementations are good enough to know what files are open

refs #5052

[exarkun]

Thanks for the excellent review.

The change linked in the above comment makes _getImplementation return the last implementation it comes across, if none of them can detect open file descriptors.

An alternate solution to the problem would be to hard-code _fallbackFDImplementation, but I like the change in r32439 because it keeps _getImplementation indifferent to the actual implementation methods. However, it does add a little trick to the _implementations list, as something like _fallbackFDImplementation now must appear last in the list. However, since the ordering of the list is already important, I think I can live with that. I am open to suggestions, though.

The resource module is in a try/except in case someone built Python without the resource module, I guess. I don't know if I really care about this case, but since it is already implemented and tested...

Some tests still fail on the OpenBSD slave. I think they are not implementation problems, because they are demanding more correct results than the platform can supply (I hesitate to call them test bugs because they _are_ demanding something correct ;). I think we can do another pass with some much simpler fixes to the tests to either skip the more rigorous ones on OpenBSD or loosen their requirements or something similar.

 Latest build results and another  OpenBSD build.

[glyph]

The linked change looks like it addresses the relevant issue, and the OpenBSD build looks good. My criticism wasn't that it made the build fail - that was the case before, and I expected it to be the case after the change too - it was that it introduced so many new failures. 5 failures 10 errors now (as in trunk) but it was 38 failures 192 errors with the problematic change.

Let's try this again: LGTM. Land.

[exarkun]

(In [32440]) Merge listopenfds-resource-fallback-5052

Author: soyt, exarkun Reviewer: glyph, jesstess Fixes: #5052

Change the logic which selects an implementation of the list-open-file-descriptors to allow the resource module to suggest an upper bound on platforms which have a non-working "/dev/fd".

In particular, this improves behavior on OpenBSD.