Merged revisions 81007 via svnmerge from

Jean-Paul Calderone · Jean-Paul Calderone · commit b654a6c0a034 · 2010-05-08T21:11:28.000Z
svn+ssh://pythondev@svn.python.org/python/trunk

........
  r81007 | jean-paul.calderone | 2010-05-08 16:06:02 -0400 (Sat, 08 May 2010) | 1 line

  Skip signal handler re-installation if it is not necessary.  Issue 8354.
........
diff --git a/Lib/test/test_signal.py b/Lib/test/test_signal.py
@@ -256,48 +256,121 @@ def tearDown(self):
 
 class SiginterruptTest(unittest.TestCase):
     signum = signal.SIGUSR1
-    def readpipe_interrupted(self, cb):
+
+    def setUp(self):
+        """Install a no-op signal handler that can be set to allow
+        interrupts or not, and arrange for the original signal handler to be
+        re-installed when the test is finished.
+        """
+        self._cleanups = []
+        oldhandler = signal.signal(self.signum, lambda x,y: None)
+        self.addCleanup(signal.signal, self.signum, oldhandler)
+
+
+    def tearDown(self):
+        """Run any cleanup functions which have been registered.
+        """
+        for (f, a) in self._cleanups:
+            f(*a)
+
+
+    def addCleanup(self, f, *a):
+        """Register a function to be called at the end of the test method
+        run.
+        """
+        self._cleanups.append((f, a))
+
+
+    def readpipe_interrupted(self):
+        """Perform a read during which a signal will arrive.  Return True if the
+        read is interrupted by the signal and raises an exception.  Return False
+        if it returns normally.
+        """
+        # Create a pipe that can be used for the read.  Also clean it up
+        # when the test is over, since nothing else will (but see below for
+        # the write end).
         r, w = os.pipe()
+        self.addCleanup(os.close, r)
+
+        # Create another process which can send a signal to this one to try
+        # to interrupt the read.
         ppid = os.getpid()
         pid = os.fork()
 
-        oldhandler = signal.signal(self.signum, lambda x,y: None)
-        cb()
-        if pid==0:
-            # child code: sleep, kill, sleep. and then exit,
-            # which closes the pipe from which the parent process reads
+        if pid == 0:
+            # Child code: sleep to give the parent enough time to enter the
+            # read() call (there's a race here, but it's really tricky to
+            # eliminate it); then signal the parent process.  Also, sleep
+            # again to make it likely that the signal is delivered to the
+            # parent process before the child exits.  If the child exits
+            # first, the write end of the pipe will be closed and the test
+            # is invalid.
             try:
                 time.sleep(0.2)
                 os.kill(ppid, self.signum)
                 time.sleep(0.2)
             finally:
+                # No matter what, just exit as fast as possible now.
                 exit_subprocess()
-
-        try:
+        else:
+            # Parent code.
+            # Make sure the child is eventually reaped, else it'll be a
+            # zombie for the rest of the test suite run.
+            self.addCleanup(os.waitpid, pid, 0)
+
+            # Close the write end of the pipe.  The child has a copy, so
+            # it's not really closed until the child exits.  We need it to
+            # close when the child exits so that in the non-interrupt case
+            # the read eventually completes, otherwise we could just close
+            # it *after* the test.
             os.close(w)
 
+            # Try the read and report whether it is interrupted or not to
+            # the caller.
             try:
-                d=os.read(r, 1)
+                d = os.read(r, 1)
                 return False
             except OSError, err:
                 if err.errno != errno.EINTR:
                     raise
                 return True
-        finally:
-            signal.signal(self.signum, oldhandler)
-            os.waitpid(pid, 0)
 
     def test_without_siginterrupt(self):
-        i=self.readpipe_interrupted(lambda: None)
-        self.assertEquals(i, True)
+        """If a signal handler is installed and siginterrupt is not called
+        at all, when that signal arrives, it interrupts a syscall that's in
+        progress.
+        """
+        i = self.readpipe_interrupted()
+        self.assertTrue(i)
+        # Arrival of the signal shouldn't have changed anything.
+        i = self.readpipe_interrupted()
+        self.assertTrue(i)
 
     def test_siginterrupt_on(self):
-        i=self.readpipe_interrupted(lambda: signal.siginterrupt(self.signum, 1))
-        self.assertEquals(i, True)
+        """If a signal handler is installed and siginterrupt is called with
+        a true value for the second argument, when that signal arrives, it
+        interrupts a syscall that's in progress.
+        """
+        signal.siginterrupt(self.signum, 1)
+        i = self.readpipe_interrupted()
+        self.assertTrue(i)
+        # Arrival of the signal shouldn't have changed anything.
+        i = self.readpipe_interrupted()
+        self.assertTrue(i)
 
     def test_siginterrupt_off(self):
-        i=self.readpipe_interrupted(lambda: signal.siginterrupt(self.signum, 0))
-        self.assertEquals(i, False)
+        """If a signal handler is installed and siginterrupt is called with
+        a false value for the second argument, when that signal arrives, it
+        does not interrupt a syscall that's in progress.
+        """
+        signal.siginterrupt(self.signum, 0)
+        i = self.readpipe_interrupted()
+        self.assertFalse(i)
+        # Arrival of the signal shouldn't have changed anything.
+        i = self.readpipe_interrupted()
+        self.assertFalse(i)
+
+
 
 class ItimerTest(unittest.TestCase):
     def setUp(self):
diff --git a/Misc/NEWS b/Misc/NEWS
@@ -73,6 +73,9 @@ Library
 - Issue #4687: Fix accuracy of garbage collection runtimes displayed with
   gc.DEBUG_STATS.
 
+- Issue #8354: The siginterrupt setting is now preserved for all signals,
+  not just SIGCHLD.
+
 - Issue #8577: distutils.sysconfig.get_python_inc() now makes a difference 
   between the build dir and the source dir when looking for "python.h" or
   "Include".
diff --git a/Modules/signalmodule.c b/Modules/signalmodule.c
@@ -192,7 +192,12 @@ signal_handler(int sig_num)
 		return;
 	}
 #endif
+#ifndef HAVE_SIGACTION
+	/* If the handler was not set up with sigaction, reinstall it.  See
+	 * Python/pythonrun.c for the implementation of PyOS_setsig which
+	 * makes this true.  See also issue8354. */
 	PyOS_setsig(sig_num, signal_handler);
+#endif
 }
 
 
diff --git a/Python/pythonrun.c b/Python/pythonrun.c
@@ -1867,6 +1867,10 @@ PyOS_sighandler_t
 PyOS_setsig(int sig, PyOS_sighandler_t handler)
 {
 #ifdef HAVE_SIGACTION
+	/* Some code in Modules/signalmodule.c depends on sigaction() being
+	 * used here if HAVE_SIGACTION is defined.  Fix that if this code
+	 * changes to invalidate that assumption.
+	 */
 	struct sigaction context, ocontext;
 	context.sa_handler = handler;
 	sigemptyset(&context.sa_mask);

Original file line number	Diff line number	Diff line change
`@@ -192,7 +192,12 @@ signal_handler(int sig_num)`
`192`	`192`	`return;`
`193`	`193`	`}`
`194`	`194`	`#endif`
	`195`	`+#ifndef HAVE_SIGACTION`
	`196`	`+ /* If the handler was not set up with sigaction, reinstall it. See`
	`197`	`+ * Python/pythonrun.c for the implementation of PyOS_setsig which`
	`198`	`+ * makes this true. See also issue8354. */`
`195`	`199`	`PyOS_setsig(sig_num, signal_handler);`
	`200`	`+#endif`
`196`	`201`	`}`
`197`	`202`
`198`	`203`