From c5789a95cf273dcc699fa6e58045022d41982f38 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sat, 28 Sep 2024 03:41:58 -0400 Subject: [PATCH 1/5] gh-124872: Back up exception before calling PyContext_WatchCallback I believe that the value of a simpler API (and defense against poorly written callbacks) outweighs the cost of backing up and restoring the thread's exception state. --- Doc/c-api/contextvars.rst | 9 +++------ Include/cpython/context.h | 3 +++ Lib/test/test_capi/test_watchers.py | 10 ++++++++++ Python/context.c | 2 ++ 4 files changed, 18 insertions(+), 6 deletions(-) diff --git a/Doc/c-api/contextvars.rst b/Doc/c-api/contextvars.rst index 8eba54a80dc80d..0584a078ea23fa 100644 --- a/Doc/c-api/contextvars.rst +++ b/Doc/c-api/contextvars.rst @@ -141,16 +141,13 @@ Context object management functions: Context object watcher callback function. The object passed to the callback is event-specific; see :c:type:`PyContextEvent` for details. + Any pending exception is cleared before the callback is called and restored + after the callback returns. + If the callback returns with an exception set, it must return ``-1``; this exception will be printed as an unraisable exception using :c:func:`PyErr_FormatUnraisable`. Otherwise it should return ``0``. - There may already be a pending exception set on entry to the callback. In - this case, the callback should return ``0`` with the same exception still - set. This means the callback may not call any other API that can set an - exception unless it saves and clears the exception state first, and restores - it before returning. - .. versionadded:: 3.14 diff --git a/Include/cpython/context.h b/Include/cpython/context.h index 3c9be7873b9399..1443a1de1ced3c 100644 --- a/Include/cpython/context.h +++ b/Include/cpython/context.h @@ -49,6 +49,9 @@ typedef enum { * Context object watcher callback function. The object passed to the callback * is event-specific; see PyContextEvent for details. * + * Any pending exception is cleared before the callback is called and restored + * after the callback returns. + * * if the callback returns with an exception set, it must return -1. Otherwise * it should return 0 */ diff --git a/Lib/test/test_capi/test_watchers.py b/Lib/test/test_capi/test_watchers.py index f21d2627c6094b..ec3fd0c64160a5 100644 --- a/Lib/test/test_capi/test_watchers.py +++ b/Lib/test/test_capi/test_watchers.py @@ -640,6 +640,16 @@ def _in_context(stack): ctx.run(_in_context, stack) self.assertEqual(str(cm.unraisable.exc_value), "boom!") + def test_exception_save(self): + with self.context_watcher(2): + with catch_unraisable_exception() as cm: + def _in_context(): + raise RuntimeError("test") + + with self.assertRaisesRegex(RuntimeError, "test"): + contextvars.copy_context().run(_in_context) + self.assertEqual(str(cm.unraisable.exc_value), "boom!") + def test_clear_out_of_range_watcher_id(self): with self.assertRaisesRegex(ValueError, r"Invalid context watcher ID -1"): _testcapi.clear_context_watcher(-1) diff --git a/Python/context.c b/Python/context.c index 8bc487a33c890b..b9eca4a355b293 100644 --- a/Python/context.c +++ b/Python/context.c @@ -125,11 +125,13 @@ notify_context_watchers(PyThreadState *ts, PyContextEvent event, PyObject *ctx) if (bits & 1) { PyContext_WatchCallback cb = interp->context_watchers[i]; assert(cb != NULL); + PyObject *exc = _PyErr_GetRaisedException(ts); if (cb(event, ctx) < 0) { PyErr_FormatUnraisable( "Exception ignored in %s watcher callback for %R", context_event_name(event), ctx); } + _PyErr_SetRaisedException(ts, exc); } i++; bits >>= 1; From 4c115ab49233f00af46858b5514a93cdd388968b Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Mon, 14 Oct 2024 00:17:51 -0400 Subject: [PATCH 2/5] gh-124872: Clarify expected PyContext_WatchCallback return behavior --- Doc/c-api/contextvars.rst | 7 ++++--- Include/cpython/context.h | 5 +++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/Doc/c-api/contextvars.rst b/Doc/c-api/contextvars.rst index 0584a078ea23fa..528fa778fa95d5 100644 --- a/Doc/c-api/contextvars.rst +++ b/Doc/c-api/contextvars.rst @@ -144,9 +144,10 @@ Context object management functions: Any pending exception is cleared before the callback is called and restored after the callback returns. - If the callback returns with an exception set, it must return ``-1``; this - exception will be printed as an unraisable exception using - :c:func:`PyErr_FormatUnraisable`. Otherwise it should return ``0``. + If the callback raises an exception it must return ``-1``; the exception + will be printed as an unraisable exception using + :c:func:`PyErr_FormatUnraisable` and discarded. Otherwise it must return + ``0``. .. versionadded:: 3.14 diff --git a/Include/cpython/context.h b/Include/cpython/context.h index 1443a1de1ced3c..ff4833b403c22c 100644 --- a/Include/cpython/context.h +++ b/Include/cpython/context.h @@ -52,8 +52,9 @@ typedef enum { * Any pending exception is cleared before the callback is called and restored * after the callback returns. * - * if the callback returns with an exception set, it must return -1. Otherwise - * it should return 0 + * If the callback raises an exception it must return -1; the exception will be + * printed as an unraisable exception using PyErr_FormatUnraisable and + * discarded. Otherwise it must return 0. */ typedef int (*PyContext_WatchCallback)(PyContextEvent, PyObject *); From 300f2fcceacb0d7b31b6e8330053398b6d0cdc7e Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sat, 28 Sep 2024 03:47:13 -0400 Subject: [PATCH 3/5] gh-124872: Change PyContext_WatchCallback return type to void The callback cannot prevent the event from occurring by raising an exception. Nor should it be able to; any failure to switch contexts would be difficult if not impossible to handle correctly. The API should reflect that the callback is purely informational, and that the context switch will happen even if the callback doesn't want it to. --- Doc/c-api/contextvars.rst | 8 +++----- Include/cpython/context.h | 7 +++---- Modules/_testcapi/watchers.c | 19 ++++++++----------- Python/context.c | 3 ++- 4 files changed, 16 insertions(+), 21 deletions(-) diff --git a/Doc/c-api/contextvars.rst b/Doc/c-api/contextvars.rst index 528fa778fa95d5..e72f0a4faf8bef 100644 --- a/Doc/c-api/contextvars.rst +++ b/Doc/c-api/contextvars.rst @@ -136,7 +136,7 @@ Context object management functions: .. versionadded:: 3.14 -.. c:type:: int (*PyContext_WatchCallback)(PyContextEvent event, PyObject *obj) +.. c:type:: void (*PyContext_WatchCallback)(PyContextEvent event, PyObject *obj) Context object watcher callback function. The object passed to the callback is event-specific; see :c:type:`PyContextEvent` for details. @@ -144,10 +144,8 @@ Context object management functions: Any pending exception is cleared before the callback is called and restored after the callback returns. - If the callback raises an exception it must return ``-1``; the exception - will be printed as an unraisable exception using - :c:func:`PyErr_FormatUnraisable` and discarded. Otherwise it must return - ``0``. + If the callback raises an exception it will be printed as an unraisable + exception using :c:func`PyErr_FormatUnraisable` and discarded. .. versionadded:: 3.14 diff --git a/Include/cpython/context.h b/Include/cpython/context.h index ff4833b403c22c..4f8f5f5d3ea2c0 100644 --- a/Include/cpython/context.h +++ b/Include/cpython/context.h @@ -52,11 +52,10 @@ typedef enum { * Any pending exception is cleared before the callback is called and restored * after the callback returns. * - * If the callback raises an exception it must return -1; the exception will be - * printed as an unraisable exception using PyErr_FormatUnraisable and - * discarded. Otherwise it must return 0. + * If the callback raises an exception it will be printed as an unraisable + * exception using PyErr_FormatUnraisable and discarded. */ -typedef int (*PyContext_WatchCallback)(PyContextEvent, PyObject *); +typedef void (*PyContext_WatchCallback)(PyContextEvent, PyObject *); /* * Register a per-interpreter callback that will be invoked for context object diff --git a/Modules/_testcapi/watchers.c b/Modules/_testcapi/watchers.c index b4233d07134aea..1124e1a44d5295 100644 --- a/Modules/_testcapi/watchers.c +++ b/Modules/_testcapi/watchers.c @@ -629,7 +629,7 @@ static int context_watcher_ids[NUM_CONTEXT_WATCHERS] = {-1, -1}; static int num_context_object_enter_events[NUM_CONTEXT_WATCHERS] = {0, 0}; static int num_context_object_exit_events[NUM_CONTEXT_WATCHERS] = {0, 0}; -static int +static void handle_context_watcher_event(int which_watcher, PyContextEvent event, PyObject *ctx) { if (event == Py_CONTEXT_EVENT_ENTER) { num_context_object_enter_events[which_watcher]++; @@ -638,30 +638,27 @@ handle_context_watcher_event(int which_watcher, PyContextEvent event, PyObject * num_context_object_exit_events[which_watcher]++; } else { - return -1; + Py_UNREACHABLE(); } - return 0; } -static int +static void first_context_watcher_callback(PyContextEvent event, PyObject *ctx) { - return handle_context_watcher_event(0, event, ctx); + handle_context_watcher_event(0, event, ctx); } -static int +static void second_context_watcher_callback(PyContextEvent event, PyObject *ctx) { - return handle_context_watcher_event(1, event, ctx); + handle_context_watcher_event(1, event, ctx); } -static int +static void noop_context_event_handler(PyContextEvent event, PyObject *ctx) { - return 0; } -static int +static void error_context_event_handler(PyContextEvent event, PyObject *ctx) { PyErr_SetString(PyExc_RuntimeError, "boom!"); - return -1; } static PyObject * diff --git a/Python/context.c b/Python/context.c index b9eca4a355b293..a9c9aca63aeca0 100644 --- a/Python/context.c +++ b/Python/context.c @@ -126,7 +126,8 @@ notify_context_watchers(PyThreadState *ts, PyContextEvent event, PyObject *ctx) PyContext_WatchCallback cb = interp->context_watchers[i]; assert(cb != NULL); PyObject *exc = _PyErr_GetRaisedException(ts); - if (cb(event, ctx) < 0) { + cb(event, ctx); + if (_PyErr_Occurred(ts) != NULL) { PyErr_FormatUnraisable( "Exception ignored in %s watcher callback for %R", context_event_name(event), ctx); From f177ad375ccfed19210222dbc0756eaea56c8511 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Fri, 22 Nov 2024 19:00:23 -0500 Subject: [PATCH 4/5] fixup! gh-124872: Change PyContext_WatchCallback return type to void --- .../next/C_API/2024-11-22-18-58-20.gh-issue-124872.o2Pl5p.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 Misc/NEWS.d/next/C_API/2024-11-22-18-58-20.gh-issue-124872.o2Pl5p.rst diff --git a/Misc/NEWS.d/next/C_API/2024-11-22-18-58-20.gh-issue-124872.o2Pl5p.rst b/Misc/NEWS.d/next/C_API/2024-11-22-18-58-20.gh-issue-124872.o2Pl5p.rst new file mode 100644 index 00000000000000..ac9afdbfeac37a --- /dev/null +++ b/Misc/NEWS.d/next/C_API/2024-11-22-18-58-20.gh-issue-124872.o2Pl5p.rst @@ -0,0 +1,3 @@ +Callbacks registered with :c:func:`PyContext_AddWatcher` (type +:c:type:`PyContext_WatchCallback`) now return ``void`` instead of ``int``, and +no longer need to back up and restore exception state. From f6562b15cf9f074e8876a5afc6492eb8f41ddf6d Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Fri, 22 Nov 2024 19:06:27 -0500 Subject: [PATCH 5/5] fixup! gh-124872: Change PyContext_WatchCallback return type to void --- Doc/c-api/contextvars.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doc/c-api/contextvars.rst b/Doc/c-api/contextvars.rst index e72f0a4faf8bef..a1e06221bd6e9e 100644 --- a/Doc/c-api/contextvars.rst +++ b/Doc/c-api/contextvars.rst @@ -145,7 +145,7 @@ Context object management functions: after the callback returns. If the callback raises an exception it will be printed as an unraisable - exception using :c:func`PyErr_FormatUnraisable` and discarded. + exception using :c:func:`PyErr_FormatUnraisable` and discarded. .. versionadded:: 3.14