Opened 8 years ago

Closed 7 years ago

#4221 defect closed fixed (fixed)

test_acceptOutOfFiles mishandles particularly low-fd-availability conditions poorly

Reported by: Jean-Paul Calderone Owned by:
Priority: normal Milestone:
Component: core Keywords: tests
Cc: jesstess Branch: branches/less-confusing-accept-emfile-test-4221
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

test_acceptOutOfFiles accidentally closes the port it's going to call accept on if the very first non-port socket it tries to create cannot be created because available file descriptors have already been exhausted. This results in a weird failure later on. We probably can't easily make the test pass in such a low-fd-availability case, but we can make it fail less confusingly.

Change History (11)

comment:1 Changed 8 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/less-confusing-accept-emfile-test-4221

(In [28005]) Branching to 'less-confusing-accept-emfile-test-4221'

comment:2 Changed 8 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted

Yea. I did it.

With a broken version of pygtk, with this branch and the #3431 branch merged together into trunk, test_acceptOutOfFiles fails like this:

[ERROR]: twisted.test.test_tcp_internals.PlatformAssumptionsTestCase.test_acceptOutOfFiles

Traceback (most recent call last):
  File "/home/exarkun/Projects/Twisted/trunk/twisted/test/test_tcp_internals.py", line 74, in test_acceptOutOfFiles
    client = self.socket()
  File "/home/exarkun/Projects/Twisted/trunk/twisted/test/test_tcp_internals.py", line 57, in socket
    s = socket.socket()
  File "/usr/lib/python2.6/socket.py", line 182, in __init__
    _sock = _realsocket(family, type, proto)
socket.error: [Errno 24] Too many open files

This is an improvement over how it fails in the #3431 branch:

[FAIL]: twisted.test.test_tcp_internals.PlatformAssumptionsTestCase.test_acceptOutOfFiles

Traceback (most recent call last):
  File "/home/exarkun/Projects/Twisted/trunk/twisted/test/test_tcp_internals.py", line 97, in test_acceptOutOfFiles
    self.assertIn(exc.args[0], (EMFILE, ENOBUFS))
twisted.trial.unittest.FailTest: 9 not in (24, 105)

}}}

comment:3 Changed 8 years ago by jesstess

Cc: jesstess added
Keywords: review removed
Owner: set to Jean-Paul Calderone

On my Mac running 10.6, running trial test_tcp_internals.py repeatedly will reliably Mac-equivalent-of-blue-screen-of-death my computer. It seems to take between 2 and 5 runs to do this, and it doesn't appear to happen on trunk.

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

Likewise (after I updated - before that, I couldn't reproduce it). Filed upstream, along with an example that is even less Twisted-based (ie, doesn't require any bits of Twisted at all).

The Apple bug id is 7596169, but I don't think anyone except Apple employees can do anything with that information. Let's wait a little while and see what they do, I guess.

comment:5 Changed 8 years ago by Jean-Paul Calderone

For what it's worth, that Apple bug is now in the state that Apple bugs tend to get in to - I can no longer tell whether or not it has been fixed, and I probably won't receive any more information from Apple about it.

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

(In [29013]) Skip on OS X

refs #4221

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

Keywords: review added
Owner: Jean-Paul Calderone deleted

Letting the OS X kernel bug block this fix any longer is pointless. Either Apple will fix it someday or they won't. There's really nothing we can do about it.

Test is marked as skip on OS X now.

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

Okay, in 10.6.3 the OS X bug seems fixed. So skip removed. Build results. Note the Windows failures are from an old gtk bug since fixed in trunk.

comment:9 Changed 7 years ago by Glyph

Keywords: review removed
Owner: set to Jean-Paul Calderone

Looks good to me. Land it, please.

It occurs to me that this test may cause problems in a concurrent testing environment, but we'll have to burn that bridge when we come to it. Not really relevant to the review, because nothing about this ticket changes that, and I'm sure it's not the only test with that problem. Let me know if you have any thoughts on it, though.

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

Resolution: fixed
Status: newclosed

(In [29039]) Merge less-confusing-accept-emfile-test-4221

Author: exarkun Reviewer: jesstess, glyph Fixes: #4221

Change one of the platform integration tests so that when it fails, it fails with a more informative error.

Previously test_acceptOutOfFiles would fail with an EBADF if it ran out of file descriptors before it managed to allocate any clients. It did this because it would accidentally close the port it wanted to accept on.

The test is changed to allocate sockets in a different order now, so if the same condition is hit, it will fail with a boring EMFILE instead.

comment:11 Changed 7 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.