#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: |
Description
Change History (13)
comment:2 Changed 18 years ago by
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 18 years ago by
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 16 years ago by
Keywords: | review added |
---|---|
Owner: | itamarst deleted |
Priority: | high → highest |
Review in emfile-662-2
comment:5 Changed 16 years ago by
Milestone: | → Twisted-2.5 |
---|
comment:6 Changed 16 years ago by
Cc: | spiv added |
---|---|
Keywords: | review removed |
Owner: | set to Jean-Paul Calderone |
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 16 years ago by
(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 16 years ago by
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 16 years ago by
Owner: | set to Glyph |
---|---|
Status: | new → assigned |
Gyah, re-reviewing I guess.
comment:10 Changed 16 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Glyph to Jean-Paul Calderone |
Status: | assigned → 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 16 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:12 Changed 13 years ago by
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 11 years ago by
Owner: | Jean-Paul Calderone deleted |
---|