launchpad-reviewers team mailing list archive

[John A Meinel <john@xxxxxxxxxxxxxxxxx>]

John A Meinel has proposed merging lp:~jameinel/launchpad/lp-forking-serve-cleaner-childre into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jameinel/launchpad/lp-forking-serve-cleaner-childre/+merge/50031

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

-- 
https://code.launchpad.net/~jameinel/launchpad/lp-forking-serve-cleaner-childre/+merge/50031
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jameinel/launchpad/lp-forking-serve-cleaner-childre into lp:launchpad.

=== modified file 'bzrplugins/lpserve/__init__.py'
--- bzrplugins/lpserve/__init__.py	2010-11-08 22:43:58 +0000
+++ bzrplugins/lpserve/__init__.py	2011-02-16 18:42:54 +0000
@@ -424,14 +424,18 @@
 
     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:
+            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 +451,17 @@
         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.
+        # 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.
         sys.exitfunc()
         # See [Decision #6]
-        os._exit(retcode)
+        return retcode
 
     @staticmethod
     def command_to_argv(command_str):