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

[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

[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.

[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)

[exarkun]

Review in emfile-662-2

[spiv]

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.

[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

[exarkun]

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.

[glyph]

Gyah, re-reviewing I guess.

[glyph]

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.

[exarkun]

(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).

[glyph]

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