Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#7633 enhancement closed fixed (fixed)

test_openFileDescriptors in process tests fails sometimes with an extra file descriptor

Reported by: Glyph Owned by: Glyph
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/open-file-descriptors-7633
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph

Description

See for example this build.

Change History (20)

comment:1 Changed 7 years ago by Glyph

This might be a duplicate of #4747.

comment:2 Changed 7 years ago by Glyph

Author: glyph
Branch: branches/open-file-descriptors-7633

(In [43181]) Branching to open-file-descriptors-7633.

comment:3 Changed 7 years ago by Glyph

Keywords: review added
Milestone: Twisted 14.1.0

I am putting this into the milestone because we are supposed to release with green supported builders, and this is keeping one of the builders red.

I'm also putting it into review, though!

comment:4 Changed 7 years ago by Jean-Paul Calderone

Where do the extra file descriptors that cause the test to fail come from? The comment in the branch that suggests the platform might interfere with a more straightforward test should give a concrete example of where this happens.

comment:5 Changed 7 years ago by Glyph

(In [43185]) explain the cases where file descriptors are left open

refs #7633

comment:6 in reply to:  4 Changed 7 years ago by Glyph

Replying to exarkun:

Where do the extra file descriptors that cause the test to fail come from? The comment in the branch that suggests the platform might interfere with a more straightforward test should give a concrete example of where this happens.

As it happens the one that is interfering right now is /dev/urandom, but really it's a generalization of the special-case the test already had (the file descriptor left open by _listOpenFDs, which could be addressed by the implementation of that function, so isn't really part of what the test is covering). I think this is a security enhancement made on Python 2.7's development branch.

I put a comment in the branch.

comment:7 Changed 7 years ago by Jean-Paul Calderone

Just thinking out loud, I wonder if running the child process with subprocess.Popen(..., close_fds=True) first to collect a list of descriptors that are open in a child process launched by someone else's implementation and comparing that list against the descriptors that are open when we use spawnProcess (our implementing).

Presumably this still isn't flawless (the obvious drawback is that the test will fail if a bug subprocess.Popen causes it to not close some descriptors - though as of now its implementation of this feature is simpler than ours). Maybe it's still a good trade-off, maybe not.

comment:8 in reply to:  7 Changed 7 years ago by Glyph

Replying to exarkun:

Just thinking out loud, I wonder if running the child process with subprocess.Popen(..., close_fds=True) first to collect a list of descriptors that are open in a child process launched by someone else's implementation and comparing that list against the descriptors that are open when we use spawnProcess (our implementing).

Presumably this still isn't flawless (the obvious drawback is that the test will fail if a bug subprocess.Popen causes it to not close some descriptors - though as of now its implementation of this feature is simpler than ours). Maybe it's still a good trade-off, maybe not.

If we gather the expected results this way, there is also the possibility that the code generating these results is just buggy (_listOpenFDs returns an empty list, for example). I think it would be worse than what's in the branch now.

comment:9 Changed 7 years ago by Glyph

(In [43188]) Since itamar complained. refs #7633

comment:10 Changed 7 years ago by Jean-Paul Calderone

r43188 seems like a bad change to me. Apart from being unrelated to the issue in the ticket description, it changes the failure behavior of the test to be worse.

Consider:

>>> eval("oh no some junk")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<string>", line 1
    oh no some junk
        ^
SyntaxError: invalid syntax

vs

>>> json.loads("oh no some junk")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.7/json/__init__.py", line 326, in loads
    return _default_decoder.decode(s)
  File "/usr/lib/python2.7/json/decoder.py", line 365, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/lib/python2.7/json/decoder.py", line 383, in raw_decode
    raise ValueError("No JSON object could be decoded")
ValueError: No JSON object could be decoded

Which would you rather appear in a test log file you have to read on buildbot to understand a bug that arises?

If this change needs to be part of this branch, please change the test code to preserve visibility of the unparseable output from the child process.

comment:11 Changed 7 years ago by Glyph

(In [43189]) Revert "Since itamar complained. refs #7633"

comment:12 in reply to:  10 Changed 7 years ago by Glyph

Replying to exarkun:

r43188 seems like a bad change to me. Apart from being unrelated to the issue in the ticket description, it changes the failure behavior of the test to be worse.

You make a good point, I've reverted it.

comment:13 Changed 7 years ago by Glyph

I forced a new build so that the buildbot is up to date with these changes.

comment:14 Changed 7 years ago by Jonathan Lange

I haven't reviewed for style guide compliance.

  1. I think this has one 'itself' too many.
     # The call to "os.listdir()" itself that _listOpenFDs's implementation
     # uses itself needs to open a file descriptor (with "opendir"), which
     # shows up in its result.
    
  2. Given that the test is still non-deterministic, it's probably a good idea to mention that in the final assertion, pointing readers to the (great!) comment in the test itself.
  3. It might be useful to have a summary of the testing strategy near the top: "open an unlikely file descriptor in the parent and verify that it's not open in the child"

Hope this helps.

comment:15 Changed 7 years ago by Jonathan Lange

Keywords: review removed
Owner: set to Glyph

Forgot to do the reassignment step. My review is in comment:14

comment:16 Changed 7 years ago by Jonathan Lange

Please address these minor issues & merge. :)

comment:17 Changed 7 years ago by Glyph

(In [43288]) Clarify comment refs #7633

comment:18 Changed 7 years ago by Glyph

(In [43289]) More comments as per jml's review, since this test is extremely subtle refs #7633

comment:19 Changed 7 years ago by Glyph

Resolution: fixed
Status: newclosed

(In [43290]) Merge open-file-descriptors-7633: Fix intermittent test.

Author: glyph

Reviewer: jml

Fixes: #7633

test_openFileDescriptors verified too strictly that no unexpected file descriptors are open in a subprocess. Sometimes, "extra" file descriptors are in fact expected (such as when /dev/urandom is opened by importing the 'random' module). This adjusts the test to verify that *twisted* is doing its job and closing the file descriptors before the process starts, rather than verifying that the list is exactly what we expect.

comment:20 Changed 6 years ago by hawkowl

Milestone: Twisted 14.1.0

Ticket retargeted after milestone deleted

Note: See TracTickets for help on using tickets.