Opened 5 years ago

Closed 4 years ago

#4221 defect closed fixed (fixed)

test_acceptOutOfFiles mishandles particularly low-fd-availability conditions poorly

Reported by: exarkun Owned by:
Priority: normal Milestone:
Component: core Keywords: tests
Cc: jesstess Branch: branches/less-confusing-accept-emfile-test-4221
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

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 5 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/less-confusing-accept-emfile-test-4221

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

comment:2 Changed 5 years ago by exarkun

  • Keywords review added
  • Owner exarkun 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 4 years ago by jesstess

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

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 4 years ago by exarkun

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 4 years ago by exarkun

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 4 years ago by exarkun

(In [29013]) Skip on OS X

refs #4221

comment:7 Changed 4 years ago by exarkun

  • Keywords review added
  • Owner exarkun 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 4 years ago by exarkun

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 4 years ago by glyph

  • Keywords review removed
  • Owner set to exarkun

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 4 years ago by exarkun

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

(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 3 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.