Opened 13 years ago

Closed 11 years ago

Last modified 8 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:


Change History (13)

comment:1 Changed 13 years ago by jdahlin

The following patch fixes it:

--- /usr/lib/python2.3/site-packages/twisted/internet/    2004-01-26
00:32:47.000000000 +0100
+++ twisted/internet/      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 != '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
-                    elif e.args[0] == EPERM:
+                    elif e.args[0] in (EMFILE, EPERM):

comment:2 Changed 13 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 13 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')

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

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

Keywords: review added
Owner: itamarst deleted
Priority: highhighest

Review in emfile-662-2

comment:5 Changed 11 years ago by radix

Milestone: Twisted-2.5

comment:6 Changed 11 years ago by spiv

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

Here's a review for you...

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(('', serverPortNumber))
+        except socket.error, e:
+            if e.args[0] != EINPROGRESS:
+                # Uh - what?
+                raise
+        else:
+  "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.


The changes to twisted/internet/ 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 11 years ago by Jean-Paul Calderone

(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 11 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone 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 11 years ago by Glyph

Owner: set to Glyph
Status: newassigned

Gyah, re-reviewing I guess.

comment:10 Changed 11 years ago by Glyph

Keywords: review removed
Owner: changed from Glyph to Jean-Paul Calderone
Status: assignednew

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 11 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

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

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