Change the behavior of _close() when both term and kill given

tueda · Sep 17, 2015 · f2cb371 · f2cb371
1 parent 6a90bce
commit f2cb371
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 50 deletions.
diff --git a/form/formlink.py b/form/formlink.py
@@ -239,30 +239,42 @@ def _close(self, term=False, kill=False):
                 except IOError as e:
                     if e.errno != errno.EPIPE:
                         raise
+
                 if term or kill:
-                    # When a non-zero `term` or `kill` is given, we first wait
-                    # for the child to finish within the duration. If not, stop
-                    # it.
                     import signal
                     import time
-                    t = 0.0
-                    dt = 0.01
-                    timeout = max(term, kill)  # negative value -> no duration
-                    while t < timeout:
-                        pid, err = os.waitpid(self._childpid, os.WNOHANG)
-                        if pid:
-                            break
-                        time.sleep(dt)
-                        t += dt
-                    else:
-                        # Stop the FORM process. Note that we don't use
-                        # setpgrp() and killpg() for the child, but directly
-                        # kill the FORM process.
-                        # In many cases SIGTERM may be enough to stop FORM.
-                        os.kill(self._formpid,
-                                signal.SIGKILL if kill else signal.SIGTERM)
+
+                    # When a non-zero `term` or `kill` is given, we first wait
+                    # for the child to finish within the duration. If not, stop
+                    # it by SIGTERM/SIGKILL. If both `term` and `kill` are
+                    # non-zero, we first try SIGTERM, and then SIGKILL.
+                    # To stop the FORM process, we do not use setpgrp() and
+                    # killpg() for the child, but directly use kill() for the
+                    # FORM process. We expect that then the child process can
+                    # finish shortly.
+
+                    def wait(timeout):
                         # Wait for the child to finish.
-                        os.waitpid(self._childpid, 0)
+                        t = 0.0
+                        dt = 0.01
+                        while t < timeout:  # negative timeout -> no wait
+                            pid, err = os.waitpid(self._childpid, os.WNOHANG)
+                            if pid:
+                                return False
+                            time.sleep(dt)
+                            t += dt
+                        return True  # still exists
+
+                    if term and kill:
+                        if wait(term):
+                            os.kill(self._formpid, signal.SIGTERM)
+                            if wait(term):
+                                os.kill(self._formpid, signal.SIGKILL)
+                                os.waitpid(self._childpid, 0)
+                    else:
+                        if wait(max(term, kill)):
+                            os.kill(self._formpid,
+                                    signal.SIGKILL if kill else signal.SIGTERM)
                 else:
                     os.waitpid(self._childpid, 0)
                 self._parentin.close()

diff --git a/form/tests/test_form.py b/form/tests/test_form.py
@@ -316,38 +316,28 @@ def test_kill(self):
         import time
 
         with form.open() as f:
-            f.write('''
-                Auto V p;
-                L F = g_(0,p1,...,p30);
-                trace4,0;
-                .sort
-            ''')
-            f.flush()
-            time.sleep(1)
-
             def timeout_handler(signum, frame):
                 raise RuntimeError('Timeout')
 
-            signal.signal(signal.SIGALRM, timeout_handler)
-            signal.alarm(5)
-            try:
-                f._close(term=True)
-            finally:
-                signal.alarm(0)
+            def do_test(func):
+                f.write('''
+                    Auto V p;
+                    L F = g_(0,p1,...,p30);
+                    trace4,0;
+                    .sort
+                ''')
+                f.flush()
+                time.sleep(1)
 
-            f.open()
-            f.write('''
-                Auto V p;
-                L F = g_(0,p1,...,p30);
-                trace4,0;
-                .sort
-            ''')
-            f.flush()
-            time.sleep(1)
+                signal.signal(signal.SIGALRM, timeout_handler)
+                signal.alarm(5)
+                try:
+                    func()
+                finally:
+                    signal.alarm(0)
 
-            signal.signal(signal.SIGALRM, timeout_handler)
-            signal.alarm(5)
-            try:
-                f.kill()
-            finally:
-                signal.alarm(0)
+            do_test(lambda: f.kill())
+            f.open()
+            do_test(lambda: f._close(term=True))
+            f.open()
+            do_test(lambda: f._close(term=True, kill=True))