Merge lp:~jameinel/launchpad/lp-forking-serve-cleaner-childre into lp:launchpad

[John A Meinel]

[r=jcsackett,sinzui][no-qa] Force lp-forking-service children to exit on any error, rather than falling back to master code.

This is one step along the way to squashing bug #717345.

I finally was able to trigger the bug manually, and the first thing I noticed was if the children get interrupted at the right place, they revert to being a master process.

So this squashes all of those with a try/finally.

I think what was happening is that they were blocked trying to open their new stdin, and then SIGINT triggers them to wakeup and start exiting, and then they think they can run as a master process.

I did not add a test for it, because it requires a fair amount of trickery to stop the process at just the right time, and it is hard to detect. If we feel it is required, I might be able to put something together. (Request a fork, but don't bind to it, send it SIGINT, and make sure it exits with code 127. But I don't know how to check the return code of a process that isn't your direct child...)

[j.c.sackett (jcsackett)]

John--

I can't say I'm 100% comfortable with approving this, given there's no tests. fork configurations and processes have already caused on incident report, so I think we need more confidence that this works as intended before approving it.

I'm not claiming this as a review, b/c you might find someone who knows more about the problem area and can approve this with some confidence. I do urge you to look into testing options for it; that might not be test coverage, but we should see how this code works in some fashion.

[j.c.sackett (jcsackett)]

After a conversation in IRC, I realize I have misunderstood the situation. Given that, I think is fine.

6:22 PM jam
jcsacket: ... I have run the specific code quite thoroughly, and there are some tests that already cover it.
6:22 PM
It is a little bit hard to inject specific failures into that exact point. If you feel it is critical, we can try to figure something out
6:22 PM
but I did do a lot of manual testing.

6:22 PM jcsackett
jam: ah, in your MP you said there were no tests; i took that to mean no testing. :-)

6:22 PM jam
no new tests covering what changed

6:23 PM jcsackett
dig.

review: Approve (code*)

[Curtis Hovey (sinzui)]

This looks good to land.

review: Approve (code)

=== modified file 'bzrplugins/lpserve/__init__.py'
--- bzrplugins/lpserve/__init__.py	2010-11-08 22:43:58 +0000
+++ bzrplugins/lpserve/__init__.py	2011-02-17 21:50:50 +0000
@@ -424,14 +424,23 @@
 
     def become_child(self, command_argv, path):
         """We are in the spawned child code, do our magic voodoo."""
-        # Stop tracking new signals
-        self._unregister_signals()
-        # Reset the start time
-        trace._bzr_log_start_time = time.time()
-        trace.mutter('%d starting %r'
-                     % (os.getpid(), command_argv))
-        self._bind_child_file_descriptors(path)
-        self._run_child_command(command_argv)
+        retcode = 127 # Failed in a bad way, poor cleanup, etc.
+        try:
+            # Stop tracking new signals
+            self._unregister_signals()
+            # Reset the start time
+            trace._bzr_log_start_time = time.time()
+            trace.mutter('%d starting %r'
+                         % (os.getpid(), command_argv))
+            self._bind_child_file_descriptors(path)
+            retcode = self._run_child_command(command_argv)
+        finally:
+            # We force os._exit() here, because we don't want to unwind the
+            # stack, which has complex results. (We can get it to unwind back
+            # to the cmd_launchpad_forking_service code, and even back to
+            # main() reporting thereturn code, but after that, suddenly the
+            # return code changes from a '0' to a '1', with no logging of info.
+            os._exit(retcode)
 
     def _run_child_command(self, command_argv):
         # This is the point where we would actually want to do something with
@@ -447,17 +456,12 @@
         self._close_child_file_descriptors()
         trace.mutter('%d finished %r'
                      % (os.getpid(), command_argv))
-        # We force os._exit() here, because we don't want to unwind the stack,
-        # which has complex results. (We can get it to unwind back to the
-        # cmd_launchpad_forking_service code, and even back to main() reporting
-        # thereturn code, but after that, suddenly the return code changes from
-        # a '0' to a '1', with no logging of info.
-        # TODO: Should we call sys.exitfunc() here? it allows atexit functions
-        #       to fire, however, some of those may be still around from the
-        #       parent process, which we don't really want.
+        # TODO: Should we call sys.exitfunc() here? it allows atexit
+        #       functions to fire, however, some of those may be still
+        #       around from the parent process, which we don't really want.
         sys.exitfunc()
         # See [Decision #6]
-        os._exit(retcode)
+        return retcode
 
     @staticmethod
     def command_to_argv(command_str):