gh-118692: Avoid creating unnecessary StopIteration instances for mon…

…itoring (#119216)
python · May 21, 2024 · 6e9863d · 6e9863d
1 parent b641825
commit 6e9863d
Show file tree

Hide file tree

Showing 10 changed files with 60 additions and 35 deletions.
diff --git a/Doc/c-api/monitoring.rst b/Doc/c-api/monitoring.rst
@@ -121,10 +121,10 @@ See :mod:`sys.monitoring` for descriptions of the events.
    :c:func:`PyErr_GetRaisedException`).
 
 
-.. c:function:: int PyMonitoring_FireStopIterationEvent(PyMonitoringState *state, PyObject *codelike, int32_t offset)
+.. c:function:: int PyMonitoring_FireStopIterationEvent(PyMonitoringState *state, PyObject *codelike, int32_t offset, PyObject *value)
 
-   Fire a ``STOP_ITERATION`` event with the current exception (as returned by
-   :c:func:`PyErr_GetRaisedException`).
+   Fire a ``STOP_ITERATION`` event. If ``value`` is an instance of :exc:`StopIteration`, it is used. Otherwise,
+   a new :exc:`StopIteration` instance is created with ``value`` as its argument.
 
 
 Managing the Monitoring State

diff --git a/Include/cpython/monitoring.h b/Include/cpython/monitoring.h
@@ -101,7 +101,7 @@ PyAPI_FUNC(int)
 _PyMonitoring_FirePyUnwindEvent(PyMonitoringState *state, PyObject *codelike, int32_t offset);
 
 PyAPI_FUNC(int)
-_PyMonitoring_FireStopIterationEvent(PyMonitoringState *state, PyObject *codelike, int32_t offset);
+_PyMonitoring_FireStopIterationEvent(PyMonitoringState *state, PyObject *codelike, int32_t offset, PyObject *value);
 
 
 #define _PYMONITORING_IF_ACTIVE(STATE, X)  \
@@ -240,11 +240,11 @@ PyMonitoring_FirePyUnwindEvent(PyMonitoringState *state, PyObject *codelike, int
 }
 
 static inline int
-PyMonitoring_FireStopIterationEvent(PyMonitoringState *state, PyObject *codelike, int32_t offset)
+PyMonitoring_FireStopIterationEvent(PyMonitoringState *state, PyObject *codelike, int32_t offset, PyObject *value)
 {
     _PYMONITORING_IF_ACTIVE(
         state,
-        _PyMonitoring_FireStopIterationEvent(state, codelike, offset));
+        _PyMonitoring_FireStopIterationEvent(state, codelike, offset, value));
 }
 
 #undef _PYMONITORING_IF_ACTIVE
diff --git a/Include/internal/pycore_opcode_metadata.h b/Include/internal/pycore_opcode_metadata.h
diff --git a/Lib/test/test_monitoring.py b/Lib/test/test_monitoring.py
@@ -1938,19 +1938,28 @@ def setUp(self):
             ( 1, E.RAISE, capi.fire_event_raise, ValueError(2)),
             ( 1, E.EXCEPTION_HANDLED, capi.fire_event_exception_handled, ValueError(5)),
             ( 1, E.PY_UNWIND, capi.fire_event_py_unwind, ValueError(6)),
-            ( 1, E.STOP_ITERATION, capi.fire_event_stop_iteration, ValueError(7)),
+            ( 1, E.STOP_ITERATION, capi.fire_event_stop_iteration, 7),
+            ( 1, E.STOP_ITERATION, capi.fire_event_stop_iteration, StopIteration(8)),
         ]
 
+        self.EXPECT_RAISED_EXCEPTION = [E.PY_THROW, E.RAISE, E.EXCEPTION_HANDLED, E.PY_UNWIND]
 
-    def check_event_count(self, event, func, args, expected):
+
+    def check_event_count(self, event, func, args, expected, callback_raises=None):
         class Counter:
-            def __init__(self):
+            def __init__(self, callback_raises):
+                self.callback_raises = callback_raises
                 self.count = 0
+
             def __call__(self, *args):
                 self.count += 1
+                if self.callback_raises:
+                    exc = self.callback_raises
+                    self.callback_raises = None
+                    raise exc
 
         try:
-            counter = Counter()
+            counter = Counter(callback_raises)
             sys.monitoring.register_callback(TEST_TOOL, event, counter)
             if event == E.C_RETURN or event == E.C_RAISE:
                 sys.monitoring.set_events(TEST_TOOL, E.CALL)
@@ -1987,8 +1996,9 @@ def test_fire_event(self):
 
     def test_missing_exception(self):
         for _, event, function, *args in self.cases:
-            if not (args and isinstance(args[-1], BaseException)):
+            if event not in self.EXPECT_RAISED_EXCEPTION:
                 continue
+            assert args and isinstance(args[-1], BaseException)
             offset = 0
             self.codelike = _testcapi.CodeLike(1)
             with self.subTest(function.__name__):
@@ -1997,6 +2007,17 @@ def test_missing_exception(self):
                 expected = ValueError(f"Firing event {evt} with no exception set")
                 self.check_event_count(event, function, args_, expected)
 
+    def test_fire_event_failing_callback(self):
+        for expected, event, function, *args in self.cases:
+            offset = 0
+            self.codelike = _testcapi.CodeLike(1)
+            with self.subTest(function.__name__):
+                args_ = (self.codelike, offset) + tuple(args)
+                exc = OSError(42)
+                with self.assertRaises(type(exc)):
+                    self.check_event_count(event, function, args_, expected,
+                                           callback_raises=exc)
+
 
     CANNOT_DISABLE = { E.PY_THROW, E.RAISE, E.RERAISE,
                        E.EXCEPTION_HANDLED, E.PY_UNWIND }

diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-05-20-14-57-39.gh-issue-118692.Qadm7F.rst b/Misc/NEWS.d/next/Core and Builtins/2024-05-20-14-57-39.gh-issue-118692.Qadm7F.rst
@@ -0,0 +1 @@
+Avoid creating unnecessary :exc:`StopIteration` instances for monitoring.
diff --git a/Modules/_testcapi/monitoring.c b/Modules/_testcapi/monitoring.c
@@ -416,16 +416,17 @@ fire_event_stop_iteration(PyObject *self, PyObject *args)
 {
     PyObject *codelike;
     int offset;
-    PyObject *exception;
-    if (!PyArg_ParseTuple(args, "OiO", &codelike, &offset, &exception)) {
+    PyObject *value;
+    if (!PyArg_ParseTuple(args, "OiO", &codelike, &offset, &value)) {
         return NULL;
     }
-    NULLABLE(exception);
+    NULLABLE(value);
+    PyObject *exception = NULL;
     PyMonitoringState *state = setup_fire(codelike, offset, exception);
     if (state == NULL) {
         return NULL;
     }
-    int res = PyMonitoring_FireStopIterationEvent(state, codelike, offset);
+    int res = PyMonitoring_FireStopIterationEvent(state, codelike, offset, value);
     RETURN_INT(teardown_fire(res, state, exception));
 }
 

diff --git a/Python/bytecodes.c b/Python/bytecodes.c
@@ -292,11 +292,9 @@ dummy_func(
             /* Need to create a fake StopIteration error here,
              * to conform to PEP 380 */
             if (PyGen_Check(receiver)) {
-                PyErr_SetObject(PyExc_StopIteration, value);
-                if (monitor_stop_iteration(tstate, frame, this_instr)) {
+                if (monitor_stop_iteration(tstate, frame, this_instr, value)) {
                     ERROR_NO_POP();
                 }
-                PyErr_SetRaisedException(NULL);
             }
             DECREF_INPUTS();
         }
@@ -307,11 +305,9 @@ dummy_func(
 
         tier1 inst(INSTRUMENTED_END_SEND, (receiver, value -- value)) {
             if (PyGen_Check(receiver) || PyCoro_CheckExact(receiver)) {
-                PyErr_SetObject(PyExc_StopIteration, value);
-                if (monitor_stop_iteration(tstate, frame, this_instr)) {
+                if (monitor_stop_iteration(tstate, frame, this_instr, value)) {
                     ERROR_NO_POP();
                 }
-                PyErr_SetRaisedException(NULL);
             }
             Py_DECREF(receiver);
         }

diff --git a/Python/ceval.c b/Python/ceval.c
@@ -231,7 +231,8 @@ static void monitor_reraise(PyThreadState *tstate,
                  _Py_CODEUNIT *instr);
 static int monitor_stop_iteration(PyThreadState *tstate,
                  _PyInterpreterFrame *frame,
-                 _Py_CODEUNIT *instr);
+                 _Py_CODEUNIT *instr,
+                 PyObject *value);
 static void monitor_unwind(PyThreadState *tstate,
                  _PyInterpreterFrame *frame,
                  _Py_CODEUNIT *instr);
@@ -2215,12 +2216,19 @@ monitor_reraise(PyThreadState *tstate, _PyInterpreterFrame *frame,
 
 static int
 monitor_stop_iteration(PyThreadState *tstate, _PyInterpreterFrame *frame,
-                       _Py_CODEUNIT *instr)
+                       _Py_CODEUNIT *instr, PyObject *value)
 {
     if (no_tools_for_local_event(tstate, frame, PY_MONITORING_EVENT_STOP_ITERATION)) {
         return 0;
     }
-    return do_monitor_exc(tstate, frame, instr, PY_MONITORING_EVENT_STOP_ITERATION);
+    assert(!PyErr_Occurred());
+    PyErr_SetObject(PyExc_StopIteration, value);
+    int res = do_monitor_exc(tstate, frame, instr, PY_MONITORING_EVENT_STOP_ITERATION);
+    if (res < 0) {
+        return res;
+    }
+    PyErr_SetRaisedException(NULL);
+    return 0;
 }
 
 static void

diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h
diff --git a/Python/instrumentation.c b/Python/instrumentation.c
@@ -2622,7 +2622,7 @@ exception_event_teardown(int err, PyObject *exc) {
     }
     else {
         assert(PyErr_Occurred());
-        Py_DECREF(exc);
+        Py_XDECREF(exc);
     }
     return err;
 }
@@ -2712,15 +2712,17 @@ _PyMonitoring_FirePyUnwindEvent(PyMonitoringState *state, PyObject *codelike, in
 }
 
 int
-_PyMonitoring_FireStopIterationEvent(PyMonitoringState *state, PyObject *codelike, int32_t offset)
+_PyMonitoring_FireStopIterationEvent(PyMonitoringState *state, PyObject *codelike, int32_t offset, PyObject *value)
 {
     int event = PY_MONITORING_EVENT_STOP_ITERATION;
     assert(state->active);
+    assert(!PyErr_Occurred());
+    PyErr_SetObject(PyExc_StopIteration, value);
     PyObject *exc;
     if (exception_event_setup(&exc, event) < 0) {
         return -1;
     }
     PyObject *args[4] = { NULL, NULL, NULL, exc };
     int err = capi_call_instrumentation(state, codelike, offset, args, 3, event);
-    return exception_event_teardown(err, exc);
+    return exception_event_teardown(err, NULL);
 }