Stackless issue python#220: fix hard switching error handling

Add a test function to provoke switching errors, add test cases and
fix the error handling.

(cherry picked from commit 8ae5fe5)

Author: Anselm Kruis

Stackless/changelog.txt

@@ -9,7 +9,11 @@ What's New in Stackless 3.X.X?
 
 *Release date: 20XX-XX-XX*
 
-- https://github.com/stackless-dev/stackless/issues/219
+- https://github.com/stackless-dev/stackless/issues/220
+  Improve the error handling in case of failed stack transfers / hard tasklet
+  switches. Call Py_FatalError, if a clean recovery is impossible.
+
+- https://github.com/stackless-dev/stackless/issues/217
   C-API documentation update: update the names of watchdog flags to match the
   implementation.
 

Stackless/core/stacklesseval.c

@@ -907,7 +907,7 @@ eval_frame_callback(PyFrameObject *f, int exc, PyObject *retval)
     SLP_FRAME_EXECFUNC_DECREF(cf);
     slp_transfer_return(cst);
     /* should never come here */
-    assert(0);
+    Py_FatalError("Return from stack spilling failed.");
 fatal:
     SLP_STORE_NEXT_FRAME(ts, cf->f_back);
     return NULL;

Stackless/module/scheduling.c

@@ -531,6 +531,7 @@ kill_wrap_bad_guy(PyTaskletObject *prev, PyTaskletObject *bad_guy)
     if (prev->next == NULL)
         slp_current_insert(prev);
     SLP_STORE_NEXT_FRAME(ts, prev->f.frame);
+    Py_CLEAR(prev->f.frame);
     ts->st.current = prev;
     if (newval != NULL) {
         /* merge bad guy into exception */
@@ -612,8 +613,11 @@ jump_soft_to_hard(PyFrameObject *f, int exc, PyObject *retval)
     Py_DECREF(retval); /* consume ref according to protocol */
     SLP_FRAME_EXECFUNC_DECREF(f);
     slp_transfer_return(ts->st.current->cstate);
-    /* we either have an error or don't come back, so: */
-    assert(0);
+    /* We either have an error or don't come back, so bail out.
+     * There is no way to recover, because we don't know the previous
+     * tasklet.
+     */
+    Py_FatalError("Soft-to-hard tasklet switch failed.");
     return NULL;
 }
 
@@ -1013,7 +1017,9 @@ slp_schedule_task(PyObject **result, PyTaskletObject *prev, PyTaskletObject *nex
 
     fail = slp_schedule_task_prepared(ts, result, prev, next, stackless, did_switch);
     if (fail && inserted) {
-        slp_current_uninsert(next);
+        /* in case of an error, it is unknown, if the tasklet is still scheduled */
+        if (next->next)
+            slp_current_uninsert(next);
         if (u_chan)
             slp_channel_insert(u_chan, next, u_dir, u_next);
         else
@@ -1221,7 +1227,7 @@ slp_schedule_task_prepared(PyThreadState *ts, PyObject **result, PyTaskletObject
         SLP_EXCHANGE_EXCINFO(ts, next);
         SLP_EXCHANGE_EXCINFO(ts, prev);
         PyFrameObject *f = SLP_CLAIM_NEXT_FRAME(ts);
-        Py_XDECREF(f);
+        Py_XSETREF(next->f.frame, f); /* revert the Py_CLEAR(next->f.frame) */
         kill_wrap_bad_guy(prev, next);
         return -1;
     }
@@ -1445,8 +1451,7 @@ slp_tasklet_end(PyObject *retval)
             Py_DECREF(retval);
             SLP_STORE_NEXT_FRAME(ts, NULL);
             slp_transfer_return(ts->st.initial_stub); /* does not return */
-            assert(0);
-            return NULL;
+            Py_FatalError("Failed to restore the initial C-stack.");
         }
     }
 

Stackless/module/stacklessmodule.c

@@ -589,7 +589,9 @@ PyStackless_RunWatchdogEx(long timeout, int flags)
         /* we failed to switch */
         PyTaskletObject *undo = pop_watchdog(ts);
         assert(undo == old_current);
-        slp_current_unremove(undo);
+        /* In case of an error, we don't know the state */
+        if (undo->next == NULL)
+            slp_current_unremove(undo);
         return NULL;
     }
 
@@ -1004,6 +1006,7 @@ PyDoc_STRVAR(test_cframe_nr__doc__,
 "test_cframe_nr(switches) -- a builtin testing function that does nothing\n\
 but soft tasklet switching. The function will call PyStackless_Schedule_nr() for switches\n\
 times and then finish.\n\
+All remaining arguments are intentionally undocumented. Don't use them!\n\
 Usage: Cf. test_cframe().");
 
 static
@@ -1036,14 +1039,23 @@ static
 PyObject *
 test_cframe_nr(PyObject *self, PyObject *args, PyObject *kwds)
 {
-    static char *argnames[] = {"switches", NULL};
+    static char *argnames[] = {"switches", "cstate_add_addr", NULL};
     PyThreadState *ts = PyThreadState_GET();
     PyCFrameObject *cf;
     long switches;
+    PyObject *cstack = NULL;
 
-    if (!PyArg_ParseTupleAndKeywords(args, kwds, "l:test_cframe_nr",
-                                     argnames, &switches))
+    if (!PyArg_ParseTupleAndKeywords(args, kwds, "l|O!:test_cframe_nr",
+                                     argnames, &switches, &PyCStack_Type, &cstack))
         return NULL;
+    if (cstack) {
+        /* Manipulate the startaddr of a cstack to make it (in)valid.
+         * This can be used to provoke switching errors. Without such a functionality,
+         * it is not possible to test the error handling code for switching errors.
+         */
+        ((PyCStackObject*)cstack)->startaddr += switches;
+        Py_RETURN_NONE;
+    }
     cf = slp_cframe_new(test_cframe_nr_loop, 1);
     if (cf == NULL)
         return NULL;

Stackless/module/taskletobject.c

@@ -110,6 +110,10 @@ slp_current_remove_tasklet(PyTaskletObject *task)
     PyThreadState *ts = task->cstate->tstate;
     PyTaskletObject **chain = &ts->st.current, *ret, *hold;
 
+    /* Make sure the tasklet is scheduled.
+     */
+    assert(task->next != NULL);
+    assert(task->prev != NULL);
     assert(ts != NULL);
     --ts->st.runcount;
     assert(ts->st.runcount >= 0);
@@ -1039,11 +1043,13 @@ impl_tasklet_run_remove(PyTaskletObject *task, int remove)
         if (removed) {
             assert(ts->st.del_post_switch == (PyObject *)prev);
             ts->st.del_post_switch = NULL;
-            slp_current_unremove(prev);
+            if (prev->next == NULL) /* in case of an error, the state is unknown */
+                slp_current_unremove(prev);
         }
         if (inserted) {
-            /* we must undo the insertion that we did */
-            slp_current_uninsert(task);
+            /* we must undo the insertion that we did, but we don't know the state */
+            if (task->next != NULL)
+                slp_current_uninsert(task);
             Py_DECREF(task);
         }
     } else if (!switched) {

Stackless/pickling/safe_pickle.c

@@ -47,7 +47,7 @@ pickle_callback(PyFrameObject *f, int exc, PyObject *retval)
     SLP_FRAME_EXECFUNC_DECREF(f);
     slp_transfer_return(cst);
     /* never come here */
-    assert(0);
+    Py_FatalError("Return from pickle stack spilling failed.");
     return NULL;
 }
 

Stackless/unittests/support.py

@@ -46,7 +46,7 @@
 try:
     import threading
     withThreads = True
-except:
+except Exception:
     withThreads = False
 
 
@@ -735,6 +735,7 @@ def _addSkip(self, result, reason):
                 self._ran_AsTaskletTestCase_setUp = True
             super(StacklessTestCase, self)._addSkip(result, reason)
 
+
 _tc = StacklessTestCase(methodName='run')
 try:
     _tc.setUp()
@@ -780,7 +781,7 @@ def run(self, result=None):
         def helper():
             try:
                 c.send(super(AsTaskletTestCase, self).run(result))
-            except:
+            except:  # @IgnorePep8
                 c.send_throw(*sys.exc_info())
         stackless.tasklet(helper)()
         result = c.receive()

Stackless/unittests/test_transfer_errors.py

@@ -0,0 +1,111 @@
+from __future__ import absolute_import, division, print_function
+
+import unittest
+import sys
+import stackless
+import contextlib
+
+from support import test_main  # @UnusedImport
+from support import StacklessTestCase, testcase_leaks_references
+
+
+class TestStackTransferFailures(StacklessTestCase):
+    """Test the error handling of failures in slp_transfer()
+
+    In theory, slp_transfer can't fail, but in reality there bugs. Unfortunately
+    it is usually not possible to recover from slp_transfer failures. But we try to avoid
+    undefined behavior.
+
+    The test coverage is incomplete, patches are welcome. Especially failed soft-to-hard switches
+    result in Py_FatalError("Soft-to-hard tasklet switch failed.").
+    """
+    def setUp(self):
+        StacklessTestCase.setUp(self)
+
+    @contextlib.contextmanager
+    def corrupted_cstack_startaddr(self, tasklet):
+        """Increment the cstack startaddr of this tasklet.
+
+        Hard-switching to this C-stack will fail. This context is used
+        by error handling tests
+        """
+        self.assertTrue(tasklet.alive)
+        self.assertEqual(tasklet.thread_id, stackless.current.thread_id)
+        self.assertFalse(tasklet.restorable)
+        self.assertFalse(tasklet.is_current)
+        cstate = tasklet.cstate
+        self.assertNotEqual(cstate.startaddr, 0)  # not invalidated
+        self.assertIs(cstate.task, tasklet)
+        test_cframe_nr = stackless.test_cframe_nr
+        # Otherwise undocumented and only for this test.
+        # Add 1 to the startaddr of cstate
+        test_cframe_nr(1, cstate_add_addr=cstate)
+        try:
+            yield
+        finally:
+            # Restore the pprevious value
+            test_cframe_nr(-1, cstate_add_addr=cstate)
+
+    def __task(self):
+        stackless.schedule_remove(None)
+        self.tasklet_done = True
+
+    def prepare_tasklet(self, task=None):
+        if task is None:
+            task = self.__task
+
+        self.tasklet_done = False
+        t = stackless.tasklet(self.__task)()
+        sw = stackless.enable_softswitch(False)
+        try:
+            t.run()
+        finally:
+            stackless.enable_softswitch(sw)
+        return t
+
+    def end(self, t):
+        # if t.alive:
+        t.run()
+        self.assertTrue(self.tasklet_done)
+
+    @testcase_leaks_references("unknown")
+    def test_scheduler(self):
+        t = self.prepare_tasklet()
+        with self.corrupted_cstack_startaddr(t):
+            t.insert()
+            self.assertRaisesRegex(SystemError, 'bad stack reference in transfer', stackless.run)
+        self.end(t)
+    test_scheduler.enable_softswitch = False  # soft switching causes soft-to-hard
+
+    @testcase_leaks_references("unknown")
+    def test_run(self):
+        t = self.prepare_tasklet()
+        with self.corrupted_cstack_startaddr(t):
+            self.assertRaisesRegex(SystemError, 'bad stack reference in transfer', t.run)
+        self.end(t)
+    test_run.enable_softswitch = False  # soft switching causes soft-to-hard
+
+    @testcase_leaks_references("unknown")
+    def test_switch(self):
+        t = self.prepare_tasklet()
+        with self.corrupted_cstack_startaddr(t):
+            self.assertRaisesRegex(SystemError, 'bad stack reference in transfer', t.switch)
+        self.end(t)
+    test_switch.enable_softswitch = False  # soft switching causes soft-to-hard
+
+    @testcase_leaks_references("unknown")
+    def test_kill(self):
+        t = self.prepare_tasklet()
+        with self.corrupted_cstack_startaddr(t):
+            self.assertRaisesRegex(SystemError, 'bad stack reference in transfer', t.kill)
+        self.end(t)
+    test_kill.enable_softswitch = False  # soft switching causes soft-to-hard
+
+
+#///////////////////////////////////////////////////////////////////////////////
+
+if __name__ == '__main__':
+    if not sys.argv[1:]:
+        sys.argv.append('-v')
+
+    unittest.main()