Skip to content

gh-124872: Back up exception before calling PyContext_WatchCallback #124741

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 5 additions & 9 deletions Doc/c-api/contextvars.rst
Original file line number Diff line number Diff line change
Expand Up @@ -136,20 +136,16 @@ 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.

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``.
Any pending exception is cleared before the callback is called and restored
after the callback returns.

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.
If the callback raises an exception it will be printed as an unraisable
exception using :c:func`PyErr_FormatUnraisable` and discarded.

.. versionadded:: 3.14

Expand Down
9 changes: 6 additions & 3 deletions Include/cpython/context.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,13 @@ typedef enum {
* Context object watcher callback function. The object passed to the callback
* is event-specific; see PyContextEvent for details.
*
* if the callback returns with an exception set, it must return -1. Otherwise
* it should return 0
* Any pending exception is cleared before the callback is called and restored
* after the callback returns.
*
* 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
Expand Down
10 changes: 10 additions & 0 deletions Lib/test/test_capi/test_watchers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
19 changes: 8 additions & 11 deletions Modules/_testcapi/watchers.c
Original file line number Diff line number Diff line change
Expand Up @@ -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]++;
Expand All @@ -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 *
Expand Down
5 changes: 4 additions & 1 deletion Python/context.c
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,14 @@ notify_context_watchers(PyThreadState *ts, PyContextEvent event, PyObject *ctx)
if (bits & 1) {
PyContext_WatchCallback cb = interp->context_watchers[i];
assert(cb != NULL);
if (cb(event, ctx) < 0) {
PyObject *exc = _PyErr_GetRaisedException(ts);
cb(event, ctx);
if (_PyErr_Occurred(ts) != NULL) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this use cb's return value as before, rather than checking _PyErr_Occurred?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR changes the callback's return type to void to discourage the use of exceptions (since they get ignored anyway), so there is no return value to check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed, but I'm suggesting to not do that.

Also, the documentation is incorrect: "If the callback raises an exception it will be ignored."
it's not completely ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. I think that discarding this PR's second commit would address both of your concerns, though I would still like to tweak the documentation's wording a bit to be more precise.

Part of the reason I want to change the return value to void is to cement a future where context switches cannot fail. For gh-99633, context switches can happen when generators or coroutines resume or suspend, where a failure to switch would be particularly difficult to handle correctly. I want the event notification API to make it clear to developers that the callback is informational only; the context switch will happen even if they don't want it to.

(I should express this rationale in the commit message and PR description.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tweaked the wording of the documentation and the commit message. I still believe that it is better to return void so I kept that, but I can easily undo it by discarding the final commit if you disagree. Please let me know what you think.

Copy link
Member

@1st1 1st1 Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But in this case we know where the exception is coming from, _PyErr_GetRaisedException resets the current exception to NULL. So if _PyErr_Occurred returns anything it must be coming from the callback.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(FWIW I don't feel strong about this, but I do like the idea somewhat)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iritkatriel Does the thumbs-up reaction on @1st1's comment mean you're OK with this PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It related to the comment it's on.

I would prefer avoiding _PyErr_Occurred as a matter of style but I agree that in this case it's harmless.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ambv @pablogsal can one of you weigh in here and give a third opinion? I basically like the PR as is as I think it makes a better API this way, but I also understand the point Irit is making. Also this might be the first CPython API designed this way.

PyErr_FormatUnraisable(
"Exception ignored in %s watcher callback for %R",
context_event_name(event), ctx);
}
_PyErr_SetRaisedException(ts, exc);
}
i++;
bits >>= 1;
Expand Down
Loading