Opened 10 years ago

Closed 8 years ago

Last modified 5 years ago

#662 defect closed fixed (fixed)

twisted.internet does not handle EMFILE (Too many files)

Reported by: jdahlin Owned by:
Priority: highest Milestone: Core-2.5
Component: core Keywords:
Cc: itamarst, jdahlin, spiv Branch:
Author: Launchpad Bug:

Description


Change History (13)

comment:1 Changed 10 years ago by jdahlin

The following patch fixes it:

--- /usr/lib/python2.3/site-packages/twisted/internet/tcp.py    2004-01-26
00:32:47.000000000 +0100
+++ twisted/internet/tcp.py      2004-08-10 15:48:57.377684784 +0200
@@ -50,6 +50,7 @@
     # just be doing "from errno import WSAEALREADY as EALREADY".
     EPERM       = 10001
     EINVAL      = 10022
+    EMFILE      = 10024
     EWOULDBLOCK = 10035
     EINPROGRESS = 10036
     EALREADY    = 10037
@@ -60,6 +61,7 @@
 elif os.name != 'java':
     from errno import EPERM
     from errno import EINVAL
+    from errno import EMFILE
     from errno import EWOULDBLOCK
     from errno import EINPROGRESS
     from errno import EALREADY
@@ -620,7 +622,7 @@
                     if e.args[0] in (EWOULDBLOCK, EAGAIN):
                         self.numberAccepts = i
                         break
-                    elif e.args[0] == EPERM:
+                    elif e.args[0] in (EMFILE, EPERM):
                         continue
                     raise

comment:2 Changed 10 years ago by itamarst

We probably want to log this, and also to allow users if they so wish to be
notified  of the error so that they can e.g. close idle connections.

comment:3 Changed 10 years ago by jdahlin

Not sure if its necessary to log and notify the user. Because it's likely that
once you run into this it'll be called quite often.

---
elif e.args[0] == EMFILE:
  log.msg('Maximum number of file descriptors reached')
continue
---

Will probably suffice, I'm not sure how to notify the users (pretty new to
twisted, nevertheless its internals)

comment:4 Changed 8 years ago by exarkun

  • Keywords review added
  • Owner itamarst deleted
  • Priority changed from high to highest

Review in emfile-662-2

comment:5 Changed 8 years ago by radix

  • Milestone set to Twisted-2.5

comment:6 Changed 8 years ago by spiv

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

Here's a review for you...

test_tcp_internals.py

This module needs the copyright boilerplate and a docstring.

+        # Make a client to use to connect to the server
+        client = self.socket()
+        client.setblocking(False)
+        try:
+            client.connect(('127.0.0.1', serverPortNumber))
+        except socket.error, e:
+            if e.args[0] != EINPROGRESS:
+                # Uh - what?
+                raise
+        else:
+            self.fail("Expected non-blocking connect to fail with "
+                      "EINPROGRESS, but it succeeded.")

It's not immediately obvious why we expect EINPROGRESS here. Is it because the other side has not yet called accept? If so, expand the comment to say so explicitly. Also, I'm not sure why you aren't using assertRaises for this, in much the same way you use it for the following assertion.

+    def setUp(self):
+        self.ports = []
+        self.messages = []
+        log.addObserver(self.messages.append)
+
+
+    def tearDown(self):
+        log.removeObserver(self.messages.append)
+        return gatherResults([
+            maybeDeferred(p.stopListening)
+            for p in self.ports])

I can't help but think we could do with a bit more assistance from the test framework here -- e.g. bzr's TestCase defines an addCleanup method that can be less awkward to use than one big tearDown that needs to suit all test methods on a test case. That's not really an issue for this branch to address, though...

+    def _acceptFailureTest(self, socketErrorNumber):
+        """
+        Test behavior in the face of an exception from C{accept(3)}.
+
+        @param socketErrorNumber: The errno to simulate from accept.
+        """

This docstring could be more informative about what the behaviour it's testing for is. i.e., say that it expects that calling doRead will not raise an exception, and a that message will be logged.

+    def test_noBufferSpaceFromAccept(self):
+        """
+        Similar to L{test_tooManyFilesFromAccept}, but test the case where
+        C{accept(3)} fails with C{ENOBUFS}.
+        """
+        return self._acceptFailureTest(ENOBUFS)

It seems mildly surprising that a "no buffer space from accept" condition would be expected to log a message of "Too many open file descriptors to accept new connections.", just like EMFILE. Aren't they slightly different? I think at least it would be good for the logged message to include the errno, because it might be useful information. This is especially true given my next comment...

My man page for accept(2) (on an Ubuntu Dapper system) describes an ENFILE that I'd expect should be handled similarly, but I see no tests for it here. Also I think ENOMEM should have a similar treatment.

twisted/internet/tcp.py

The changes to twisted/internet/tcp.py look fine, modulo comments I've already made.

Overall, good work. It's always great to see tests added, particularly for tricky errors like these. If you deal with my comments I'd be happy to see this merged.

comment:7 Changed 8 years ago by exarkun

(In [18190]) address some review comments, add some more tests/fixes

  • adds copyright boilerplate to new file
  • converts manual exception handling to exception-free case and documents EINPROGRESS
  • adds more description to the behavior demanded by _acceptFailureTest
  • changes logged message to be less specific but also include the errno of the failure
  • adds tests and handling for ENFILE, ENOMEM, and ECONNABORTED

Refs #662

comment:8 Changed 8 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

Thanks for the comprehensive review. I've made most of the changes you recommended. In addition to adding tests for ENFILE and ENOMEM handling, I've also added one for ECONNABORTED. I can't think of a way to write a test along the lines of test_acceptOutOfFiles for any of these errors, however.

Due to the amount of change made since the review, putting this back up for review.

comment:9 Changed 8 years ago by glyph

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

Gyah, re-reviewing I guess.

comment:10 Changed 8 years ago by glyph

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

No module docstring on test_tcp_internals. :-)

Add one, if you like. Then merge. Looks good.

By the way, is there a separate ticket for making this notification visible to application code so that existing connections can be dropped rather than refusing the new one? If not, there should be.

comment:11 Changed 8 years ago by exarkun

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

(In [18256]) Merge emfile-662-2

Author: exarkun
Reviewer: spiv, glyph
Fixes #662

Add error handling for a handful of possible errors from accept(2).

comment:12 Changed 5 years ago by glyph

Couldn't find a reference to the ticket I asked for (making this notification visible to application code) so I filed one: #3958.

comment:13 Changed 3 years ago by <automation>

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