Merge lp:~jameinel/launchpad/twisted-close-on-failure into lp:launchpad

[John A Meinel]

[r=bac,benji][ui=none] [r=bac,benji][bug=717345][no-qa] If we fail to open all handles, close the ones we did succeed in opening. (bug #717345)

This is the final step I've put together for bug #717345.

This changes the spawning code so that it more directly handles when we fail to actually connect to a newly spawned child. This should decrease the chance of leaking file handles. And with https://code.launchpad.net/~jameinel/launchpad/lp-serve-child-hangup/+merge/50055 I see that even after forcing the server to run out of handles for a while, it recovers and all remaining handles get closed.

I tested this using 'ulimit -n 100', and then 'hammer_ssh.py --load=20'. So it starts running out of handles, but serves what it can on the remaining connections. Then once the load is gone, I can see all the remaining handles get closed, and the process is back to a clean state.

[Benji York (benji)]

This branch looks like it will greatly help prevent resource exhaustion.
Approved* with a couple of questions/observations for your consideration:

(* I'm doing mentored reviews, so Brad will be reviewing my review and
giving final approval.)

I'm curious why the check (on line 26 of the diff) that the content of
"response" not start with "FAILURE" was removed? Was it because the
assert will also fire in that case? If so, note two things: 1) that's
an abuse of assert in my book: assert is for asserting the programmer
beliefs about preconditions, postconditions, and invariants, not
checking for erroneous inputs, 2) asserts are compiled out if run with
-O or -OO, so it sets up a failure mode that won't be easy for someone
to track down.

The original "assert ok == 'ok'" was fine because there was a previous
check for ok == 'FAILURE' and the intent was that the only two possible
values for ok were "ok" and "FAILURE" so at that point in the code any
value other than "ok" signaled a mismatch between the programmer's
internal model and reality.

This bit of code confuses me (it's the body of openHandleFailures sans
docstring):

    fd = os.open(path, flags)
    call_on_failure.append((os.close, fd))
    p = proc_class(reactor, self, child_fd, fd)
    # The ProcessReader/Writer now handles this fileno, don't close
    # directly anymore
    call_on_failure[-1] = (p.connectionLost, (None,))
    self.pipes[child_fd] = p

In particular, it appends a tuple to call_on_failure, makes a function
call that appears to be unrelated to call_on_failure and then replaces
the just-added item at the end of call_on_failure with a new tuple.

Oh wait! Is this because the responsibility for closing fd moves to p
after it is successfully created? Is that what the comment is trying to
tell me? If so, maybe a different phrasing would help. Something like:

    # Now that p has been successfully created it takes on the
    # responsibility of closing fd, so we'll replace the explicit close
    # of fp with the connectionLost handler of p.

In the following code (lines 88 to 98 of the diff), is it possible that
any of the functions in call_on_failure might raise an exception? If
so, is another layer of try/excepts prudent?

    try:
        ...
    except:
        for func, args in call_on_failure:
            func(*args)
        raise

review: Approve (code*)

[John A Meinel (jameinel)]

The FAILURE check was removed because the code we called already checked for it.

The code was, roughly:

def _sendRequest(...):

  response = socket.recv()
  if response.startswith("FAILURE"):
    # raise exception
  return response

def _spawn(...):

  response = self._sendRequest()
  if response.startswith('FAILURE'):
    # raise exception

So I was just removing duplication.

Yes, we no longer close the fd, because the ProcessWriter/ProcessReader will close it. I can switch the comment if you find it easier to understand.

For the cleanup code... we could suppress all exceptions at that point, I guess. Its a little ugly, but that may not be preventable. FWIW, bzr has cleanup code for this sort of thing, except it always runs the cleanups (but suppresses failures if there is a current exception running.)

[Benji York (benji)]

On Fri, Feb 18, 2011 at 11:03 AM, John A Meinel <email address hidden> wrote:
> The FAILURE check was removed because the code we called already
> checked for it.
[...]
> So I was just removing duplication.

Ah! That makes sense.

> Yes, we no longer close the fd, because the
> ProcessWriter/ProcessReader will close it. I can switch the comment if
> you find it easier to understand.

I do find it clearer. That being said, I eventually figured it out so
the original comment can't be all bad.

> For the cleanup code... we could suppress all exceptions at that
> point, I guess. Its a little ugly, but that may not be preventable.
> FWIW, bzr has cleanup code for this sort of thing, except it always
> runs the cleanups (but suppresses failures if there is a current
> exception running.)

Right, the central question is: should a failure of the cleanup code
cause an exception to bubble up or should it be silently ignored. It's
possible that that failure could cause file handles to be leaked, which
is what you're trying hard to prevent. On the other hand, a rare
failure when cleaning up probably shouldn't cause the entire process to
end, so ignoring exceptions from cleanup code seems like the lesser
evil.
--
Benji York

[Brad Crittenden (bac)]

I concur with Benji's review. I had the same questions regarding the asserts and think Benji's suggestion for rephrasing the comments makes the code more clear.

The code does look to be much more robust than what we currently have.

I'll mark this branch mentored (approved) and let you and Benji sort out the questions he raised.

review: Approve (code)

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2/18/2011 10:07 AM, Benji York wrote:
> On Fri, Feb 18, 2011 at 11:03 AM, John A Meinel <email address hidden> wrote:
>> The FAILURE check was removed because the code we called already
>> checked for it.
> [...]
>> So I was just removing duplication.
>
> Ah! That makes sense.
>
>> Yes, we no longer close the fd, because the
>> ProcessWriter/ProcessReader will close it. I can switch the comment if
>> you find it easier to understand.
>
> I do find it clearer. That being said, I eventually figured it out so
> the original comment can't be all bad.

Done.

>
>> For the cleanup code... we could suppress all exceptions at that
>> point, I guess. Its a little ugly, but that may not be preventable.
>> FWIW, bzr has cleanup code for this sort of thing, except it always
>> runs the cleanups (but suppresses failures if there is a current
>> exception running.)
>
> Right, the central question is: should a failure of the cleanup code
> cause an exception to bubble up or should it be silently ignored. It's
> possible that that failure could cause file handles to be leaked, which
> is what you're trying hard to prevent. On the other hand, a rare
> failure when cleaning up probably shouldn't cause the entire process to
> end, so ignoring exceptions from cleanup code seems like the lesser
> evil.

I changed it to trap the original exception using sys.exc_info(), then
iterate the cleanups, and if there is another exception just call log.err()
Then raise the original exception again.

Which seemed a reasonable tradeoff.

Committed and pushed.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk1em8QACgkQJdeBCYSNAAOipACeJJOl3BSzYoRelLS2JrSGt9YU
4xgAnR/lWKNZutRqJB4enGW9Q/WOALQU
=+lBJ
-----END PGP SIGNATURE-----

[Benji York (benji)]

Looks good!

=== modified file 'lib/lp/codehosting/sshserver/session.py'
--- lib/lp/codehosting/sshserver/session.py	2010-10-07 23:43:40 +0000
+++ lib/lp/codehosting/sshserver/session.py	2011-02-18 16:23:00 +0000
@@ -11,6 +11,7 @@
 import os
 import signal
 import socket
+import sys
 import urlparse
 
 from zope.event import notify
@@ -21,7 +22,8 @@
     interfaces,
     process,
     )
-from twisted.python import log
+from twisted.python import log, failure
+from twisted.internet.main import CONNECTION_LOST, CONNECTION_DONE
 
 from canonical.config import config
 from lp.codehosting import get_bzr_path
@@ -129,6 +131,7 @@
             log.err('Connection failed: %s' % (e,))
             raise
         if response.startswith("FAILURE"):
+            client_sock.close()
             raise RuntimeError('Failed to send message: %r' % (response,))
         return response, client_sock
 
@@ -154,36 +157,62 @@
         message.append('end\n')
         message = ''.join(message)
         response, sock = self._sendMessageToService(message)
-        if response.startswith('FAILURE'):
-            # TODO: Is there a better error to raise?
-            raise RuntimeError("Failed while sending message to forking "
-                "service. message: %r, failure: %r"
-                % (message, response))
-        ok, pid, path, tail = response.split('\n')
-        assert ok == 'ok'
-        assert tail == ''
-        pid = int(pid)
-        log.msg('Forking returned pid: %d, path: %s' % (pid, path))
+        try:
+            ok, pid, path, tail = response.split('\n')
+            assert ok == 'ok'
+            assert tail == ''
+            pid = int(pid)
+            log.msg('Forking returned pid: %d, path: %s' % (pid, path))
+        except:
+            sock.close()
+            raise
         return pid, path, sock
 
+    def _openHandleFailures(self, call_on_failure, path, flags, proc_class,
+                            reactor, child_fd):
+        """Open the given path, adding a cleanup as appropriate.
+
+        :param call_on_failure: A list holding (callback, args) tuples. We will
+            append new entries for things that we open
+        :param path: The path to open
+        :param flags: Flags to pass to os.open
+        :param proc_class: The ProcessWriter/ProcessReader class to wrap this
+            connection.
+        :param reactor: The Twisted reactor we are connecting to.
+        :param child_fd: The child file descriptor number passed to proc_class
+        """
+        fd = os.open(path, flags)
+        call_on_failure.append((os.close, fd))
+        p = proc_class(reactor, self, child_fd, fd)
+        # Now that p has been created, it will close fd for us. So switch the
+        # cleanup to calling p.connectionLost()
+        call_on_failure[-1] = (p.connectionLost, (None,))
+        self.pipes[child_fd] = p
+
     def _connectSpawnToReactor(self, reactor):
+        self._exiter = _WaitForExit(reactor, self, self.process_sock)
+        call_on_failure = [(self._exiter.connectionLost, (None,))]
         stdin_path = os.path.join(self._fifo_path, 'stdin')
         stdout_path = os.path.join(self._fifo_path, 'stdout')
         stderr_path = os.path.join(self._fifo_path, 'stderr')
-        child_stdin_fd = os.open(stdin_path, os.O_WRONLY)
-        self.pipes[0] = process.ProcessWriter(reactor, self, 0,
-                                              child_stdin_fd)
-        child_stdout_fd = os.open(stdout_path, os.O_RDONLY)
-        # forceReadHack=True ? Used in process.py doesn't seem to be needed
-        # here
-        self.pipes[1] = process.ProcessReader(reactor, self, 1,
-                                              child_stdout_fd)
-        child_stderr_fd = os.open(stderr_path, os.O_RDONLY)
-        self.pipes[2] = process.ProcessReader(reactor, self, 2,
-                                              child_stderr_fd)
-        # Note: _exiter forms a GC cycle, since it points to us, and we hold a
-        # reference to it
-        self._exiter = _WaitForExit(reactor, self, self.process_sock)
+        try:
+            self._openHandleFailures(call_on_failure, stdin_path, os.O_WRONLY,
+                process.ProcessWriter, reactor, 0)
+            self._openHandleFailures(call_on_failure, stdout_path, os.O_RDONLY,
+                process.ProcessReader, reactor, 1)
+            self._openHandleFailures(call_on_failure, stderr_path, os.O_RDONLY,
+                process.ProcessReader, reactor, 2)
+        except:
+            exc_class, exc_value, exc_tb = sys.exc_info()
+            for func, args in call_on_failure:
+                try:
+                    func(*args)
+                except:
+                    # Just log any exceptions at this point. This makes sure
+                    # all cleanups get called so we don't get leaks. We know
+                    # there is an active exception, or we wouldn't be here.
+                    log.err()
+            raise exc_class, exc_value, exc_tb
         self.pipes['exit'] = self._exiter
 
     def _getReason(self, status):
@@ -248,12 +277,14 @@
     # Implemented because ProcessWriter/ProcessReader want to call it
     # Copied from twisted.internet.Process
     def childConnectionLost(self, childFD, reason):
-        close = getattr(self.pipes[childFD], 'close', None)
-        if close is not None:
-            close()
-        else:
-            os.close(self.pipes[childFD].fileno())
-        del self.pipes[childFD]
+        pipe = self.pipes.get(childFD)
+        if pipe is not None:
+            close = getattr(pipe, 'close', None)
+            if close is not None:
+                close()
+            else:
+                os.close(self.pipes[childFD].fileno())
+            del self.pipes[childFD]
         try:
             self.proto.childConnectionLost(childFD)
         except: