Opened 4 years ago

Closed 3 years ago

#5052 enhancement closed fixed (fixed)

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

Reported by: soyt Owned by: exarkun
Priority: normal Milestone:
Component: core Keywords:
Cc: gregoire.leroy@…, jesstess Branch: branches/listopenfds-resource-fallback-5052-2
(diff, github, buildbot, log)
Author: soyt, exarkun Launchpad Bug:

Description

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.

Attachments (1)

fallback.diff (2.4 KB) - added by soyt 4 years ago.

Download all attachments as: .zip

Change History (24)

Changed 4 years ago by soyt

comment:1 Changed 4 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

comment:2 Changed 3 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/listopenfds-resource-fallback-5052

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

comment:3 Changed 3 years ago by exarkun

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

refs #5052

comment:4 Changed 3 years ago by exarkun

  • Author changed from exarkun to soyt
  • Keywords review removed
  • Owner set to 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

comment:5 Changed 3 years ago by exarkun

  • Author changed from soyt to soyt, exarkun
  • Keywords review added
  • Owner exarkun deleted

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

comment:6 Changed 3 years ago by exarkun

comment:7 Changed 3 years ago by glyph

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

Reviewing.

comment:8 Changed 3 years ago by glyph

  • Keywords review removed
  • Owner changed from glyph to exarkun
  • Status changed from assigned to new

LGTM. Land.

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

comment:10 Changed 3 years ago by retenodus

  • Cc gregoire.leroy@… added

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

comment:11 Changed 3 years ago by exarkun

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

refs #5052

comment:12 Changed 3 years ago by exarkun

  • Branch changed from branches/listopenfds-resource-fallback-5052 to branches/listopenfds-resource-fallback-5052-2

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

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

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

comment:15 Changed 3 years ago by exarkun

  • Keywords review added

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.

comment:16 Changed 3 years ago by exarkun

  • Owner exarkun deleted

comment:17 follow-up: Changed 3 years ago by jesstess

  • Cc jesstess added
  • Keywords review removed
  • Owner set to exarkun

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?

comment:18 in reply to: ↑ 17 Changed 3 years ago by 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.

comment:19 Changed 3 years ago by exarkun

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

refs #5052

comment:20 Changed 3 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

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.

comment:21 Changed 3 years ago by glyph

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

comment:22 Changed 3 years ago by glyph

  • Keywords review removed
  • Owner changed from glyph to exarkun
  • Status changed from assigned to new

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.

comment:23 Changed 3 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

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

Note: See TracTickets for help on using tickets.