Add tests for, and fix, some assertion errors/failures of re-entrant …

…switches.

Here we produce this circumstance using a trace function.

CHANGES.rst

@@ -11,6 +11,7 @@
   trouble when previously the process might have continued for some
   time with a corrupt state. It is unlikely those errors occurred in
   practice.
+- Fix some assertion errors and potential bugs with re-entrant switches.
 
 
 3.0.0rc1 (2023-09-01)

src/greenlet/greenlet.cpp

@@ -1605,20 +1605,30 @@ Greenlet::check_switch_allowed() const
     }
 }
 
-
+/**
+ * May run arbitrary Python code.
+ */
 OwnedObject
 Greenlet::g_switch_finish(const switchstack_result_t& err)
 {
-
     ThreadState& state = *this->thread_state();
+    // Because calling the trace function could do arbitrary things,
+    // including switching away from this greenlet and then maybe
+    // switching back, we need to capture the arguments now so that
+    // they don't change.
+    OwnedObject result;
+    if (this->args()) {
+        result <<= this->args();
+    }
+    assert(!this->args());
     try {
         // Our only caller handles the bad error case
         assert(err.status >= 0);
         assert(state.borrow_current() == this->self());
 
         if (OwnedObject tracefunc = state.get_tracefunc()) {
             g_calltrace(tracefunc,
-                        this->args() ? mod_globs->event_switch : mod_globs->event_throw,
+                        result ? mod_globs->event_switch : mod_globs->event_throw,
                         err.origin_greenlet,
                         this->self());
         }
@@ -1635,9 +1645,6 @@ Greenlet::g_switch_finish(const switchstack_result_t& err)
             throw PyErrOccurred();
         }
 
-        OwnedObject result;
-        result <<= this->switch_args;
-        assert(!this->switch_args);
         return result;
     }
     catch (const PyErrOccurred&) {

src/greenlet/tests/__init__.py

@@ -176,3 +176,28 @@ def get_expected_returncodes_for_aborted_process(self):
             0xc0000409,
         )
         return expected_exit
+
+    def run_script(self, script_name):
+        import subprocess
+        import os
+        script = os.path.join(
+            os.path.dirname(__file__),
+            script_name,
+        )
+
+        return subprocess.check_output([sys.executable, script],
+                                       encoding='utf-8',
+                                       stderr=subprocess.STDOUT)
+
+    def assertScriptRaises(self, script_name, exitcodes=None):
+        import subprocess
+        with self.assertRaises(subprocess.CalledProcessError) as exc:
+            output = self.run_script(script_name)
+            __traceback_info__ = output
+            # We're going to fail the assertion if we get here, at least
+            # preserve the output in the traceback.
+
+        if exitcodes is None:
+            exitcodes = self.get_expected_returncodes_for_aborted_process()
+        self.assertIn(exc.exception.returncode, exitcodes)
+        return exc.exception

src/greenlet/tests/fail_switch_three_greenlets.py

@@ -0,0 +1,44 @@
+"""
+Uses a trace function to switch greenlets at unexpected times.
+
+In the trace function, we switch from the current greenlet to another
+greenlet, which switches
+"""
+import greenlet
+
+g1 = None
+g2 = None
+
+switch_to_g2 = False
+
+def tracefunc(*args):
+    print('TRACE', *args)
+    global switch_to_g2
+    if switch_to_g2:
+        switch_to_g2 = False
+        g2.switch()
+    print('\tLEAVE TRACE', *args)
+
+def g1_run():
+    print('In g1_run')
+    global switch_to_g2
+    switch_to_g2 = True
+    from_parent = greenlet.getcurrent().parent.switch()
+    print('Return to g1_run')
+    print('From parent', from_parent)
+
+def g2_run():
+    #g1.switch()
+    greenlet.getcurrent().parent.switch()
+
+greenlet.settrace(tracefunc)
+
+g1 = greenlet.greenlet(g1_run)
+g2 = greenlet.greenlet(g2_run)
+
+# This switch didn't actually finish!
+# And if it did, it would raise TypeError
+# because g1_run() doesn't take any arguments.
+g1.switch(1)
+print('Back in main')
+g1.switch(2)

src/greenlet/tests/fail_switch_three_greenlets2.py

@@ -0,0 +1,39 @@
+"""
+Like fail_switch_three_greenlets, but the call into g1_run would actually be
+valid.
+"""
+import greenlet
+
+g1 = None
+g2 = None
+
+switch_to_g2 = False
+
+def tracefunc(*args):
+    print('TRACE', *args)
+    global switch_to_g2
+    if switch_to_g2:
+        switch_to_g2 = False
+        g2.switch()
+    print('\tLEAVE TRACE', *args)
+
+def g1_run():
+    print('In g1_run')
+    global switch_to_g2
+    switch_to_g2 = True
+    from_parent = greenlet.getcurrent().parent.switch()
+    print('Return to g1_run')
+    print('From parent', from_parent)
+
+def g2_run():
+    #g1.switch()
+    greenlet.getcurrent().parent.switch()
+
+greenlet.settrace(tracefunc)
+
+g1 = greenlet.greenlet(g1_run)
+g2 = greenlet.greenlet(g2_run)
+
+g1.switch()
+print('Back in main')
+g1.switch(2)

src/greenlet/tests/fail_switch_two_greenlets.py

@@ -0,0 +1,41 @@
+"""
+Uses a trace function to switch greenlets at unexpected times.
+
+In the trace function, we switch from the current greenlet to another
+greenlet, which switches
+"""
+import greenlet
+
+g1 = None
+g2 = None
+
+switch_to_g2 = False
+
+def tracefunc(*args):
+    print('TRACE', *args)
+    global switch_to_g2
+    if switch_to_g2:
+        switch_to_g2 = False
+        g2.switch()
+    print('\tLEAVE TRACE', *args)
+
+def g1_run():
+    print('In g1_run')
+    global switch_to_g2
+    switch_to_g2 = True
+    greenlet.getcurrent().parent.switch()
+    print('Return to g1_run')
+    print('Falling off end of g1_run')
+
+def g2_run():
+    g1.switch()
+    print('Falling off end of g2')
+
+greenlet.settrace(tracefunc)
+
+g1 = greenlet.greenlet(g1_run)
+g2 = greenlet.greenlet(g2_run)
+
+g1.switch()
+print('Falling off end of main')
+g2.switch()

src/greenlet/tests/test_contextvars.py

@@ -2,6 +2,7 @@
 
 import gc
 import sys
+import unittest
 
 from functools import partial
 from unittest import skipUnless
@@ -302,3 +303,7 @@ def test_contextvars_errors(self):
             let1.gr_context = None
 
         del let1
+
+
+if __name__ == '__main__':
+    unittest.main()

src/greenlet/tests/test_greenlet.py

@@ -1139,6 +1139,8 @@ def test_main_greenlet_is_greenlet(self):
 
 
 class TestBrokenGreenlets(TestCase):
+    # Tests for things that used to, or still do, terminate the interpreter.
+    # This often means doing unsavory things.
 
     def test_failed_to_initialstub(self):
         def func():
@@ -1181,23 +1183,37 @@ def func():
         self.assertEqual(runs, [1, 2, 3])
 
     def test_failed_to_slp_switch_into_running(self):
-        import subprocess
-        import os.path
+        ex = self.assertScriptRaises('fail_slp_switch.py')
 
-        script = os.path.join(
-            os.path.dirname(__file__),
-            'fail_slp_switch.py'
-        )
-
-        with self.assertRaises(subprocess.CalledProcessError) as exc:
-            subprocess.check_output([sys.executable, script],
-                                    encoding='utf-8',
-                                    stderr=subprocess.STDOUT)
-
-        ex = exc.exception
         self.assertIn('fail_slp_switch is running', ex.output)
         self.assertIn(ex.returncode, self.get_expected_returncodes_for_aborted_process())
 
+    def test_reentrant_switch_two_greenlets(self):
+        # Before we started capturing the arguments in g_switch_finish, this could crash.
+        output = self.run_script('fail_switch_two_greenlets.py')
+        self.assertIn('In g1_run', output)
+        self.assertIn('TRACE', output)
+        self.assertIn('LEAVE TRACE', output)
+        self.assertIn('Falling off end of main', output)
+        self.assertIn('Falling off end of g1_run', output)
+        self.assertIn('Falling off end of g2', output)
+
+    def test_reentrant_switch_three_greenlets(self):
+        # On debug builds of greenlet, this used to crash with an assertion error;
+        # on non-debug versions, it ran fine (which it should not do!).
+        # Now it always crashes correctly with a TypeError
+        ex = self.assertScriptRaises('fail_switch_three_greenlets.py', exitcodes=(1,))
+
+        self.assertIn('TypeError', ex.output)
+        self.assertIn('positional arguments', ex.output)
+
+    def test_reentrant_switch_three_greenlets2(self):
+        # This actually passed on debug and non-debug builds. It
+        # should probably have been triggering some debug assertions
+        # but it didn't.
+        #
+        # I think the fixes for the above test also kicked in here.
+        self.run_script('fail_switch_three_greenlets2.py')
 
 if __name__ == '__main__':
     import unittest