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

rhansen · 2024-09-29T01:41:27Z

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.

Also change the return type from int 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.

I don't think a NEWS blurb is needed because this amends a feature that is new to v3.14; the existing blurb should suffice.

cc @fried

Issue: Add c-api to set callback function on enter and exit of PyContext #119333

Issue: Replace context enter and exit events with single "context switched" event #124872

📚 Documentation preview 📚: https://cpython-previews--124741.org.readthedocs.build/

bedevere-app · 2024-09-29T01:41:34Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

rruuaanng · 2024-09-29T02:24:56Z

I think it still needs to be added because this behavior is new.

rhansen · 2024-09-29T03:22:22Z

I don't understand. What should be added beyond what is already in Misc/NEWS.d/next/C API/2024-05-21-18-28-44.gh-issue-119333.OTsYVX.rst?

Doc/c-api/contextvars.rst

Python/context.c

bedevere-app · 2024-09-29T23:20:24Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Doc/c-api/contextvars.rst

bedevere-app · 2024-09-30T03:45:49Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

bedevere-app · 2024-09-30T03:55:16Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

bedevere-app · 2024-09-30T08:15:19Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

bedevere-app · 2024-10-01T23:23:11Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

rhansen · 2024-10-01T23:30:01Z

Apologies for the force-push; I wanted to update the commit message to reference the new issue I just opened (gh-124872).

1st1 · 2024-10-09T23:49:02Z

I'm +0 for this change. Curious what @fried would say.

fried · 2024-10-10T04:34:27Z

I'm +0 for this change. Curious what @fried would say.

The existing behavior mirrors the other object event "watchers"

1st1 · 2024-10-12T21:03:00Z

Ah, I see, e.g. https://docs.python.org/3/c-api/code.html#c.PyCode_WatchCallback is similarly designed. Not going to lie, I like what @rhansen proposes more, so I'm +1 for this PR.

Maybe I can hear from @pablogsal or @vstinner what they think of this?

In the meantime, @rhansen please rebase the PR to the latest main.

…back 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.

pablogsal · 2024-10-13T13:54:08Z

I'm with @1st1 on this one (+1 to this design).

Maybe the C-API working group has some input? @iritkatriel

iritkatriel · 2024-10-13T15:16:40Z

Python/context.c

-            if (cb(event, ctx) < 0) {
+            PyObject *exc = _PyErr_GetRaisedException(ts);
+            cb(event, ctx);
+            if (_PyErr_Occurred(ts) != NULL) {


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

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.

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.

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.)

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.

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.

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

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

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.

@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.

…vior

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.

… void

rhansen requested a review from 1st1 as a code owner September 29, 2024 01:41

bedevere-app bot added the awaiting review label Sep 29, 2024

bedevere-app bot mentioned this pull request Sep 29, 2024

Add c-api to set callback function on enter and exit of PyContext #119333

Closed

kumaraditya303 requested a review from gvanrossum September 29, 2024 03:24

gvanrossum reviewed Sep 29, 2024

View reviewed changes

Doc/c-api/contextvars.rst Outdated Show resolved Hide resolved

Python/context.c Outdated Show resolved Hide resolved

rhansen force-pushed the gh-119333-exception branch from 1636f10 to 9e4ea98 Compare September 29, 2024 23:20

gvanrossum reviewed Sep 30, 2024

View reviewed changes

Doc/c-api/contextvars.rst Show resolved Hide resolved

rhansen mentioned this pull request Sep 30, 2024

gh-124872: Replace enter/exit events with "switched" #124776

Merged

rhansen force-pushed the gh-119333-exception branch from 212948c to 5f8590c Compare October 1, 2024 23:23

rhansen changed the title ~~gh-119333: Back up exception before calling PyContext_WatchCallback~~ gh-124872: Back up exception before calling PyContext_WatchCallback Oct 1, 2024

bedevere-app bot mentioned this pull request Oct 1, 2024

Replace context enter and exit events with single "context switched" event #124872

Open

ZeroIntensity added the skip news label Oct 12, 2024

pythongh-124872: Back up exception before calling PyContext_WatchCall…

c5789a9

…back 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.

rhansen force-pushed the gh-119333-exception branch from 5f8590c to ca49b94 Compare October 12, 2024 21:34

iritkatriel reviewed Oct 13, 2024

View reviewed changes

rhansen added 2 commits October 14, 2024 00:40

pythongh-124872: Clarify expected PyContext_WatchCallback return beha…

4c115ab

…vior

rhansen force-pushed the gh-119333-exception branch from ca49b94 to 300f2fc Compare October 14, 2024 05:30

rhansen mentioned this pull request Nov 22, 2024

gh-127124: Pass optional state to context watcher callback #127140

Open

rhansen added 2 commits November 22, 2024 19:00

fixup! pythongh-124872: Change PyContext_WatchCallback return type to…

f177ad3

… void

fixup! pythongh-124872: Change PyContext_WatchCallback return type to…

f6562b1

… void

rhansen mentioned this pull request Nov 25, 2024

gh-127124: Change context watcher callback to a callable object #127247

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rhansen commented Sep 29, 2024 •

edited

Loading

bedevere-app bot commented Sep 29, 2024

rruuaanng commented Sep 29, 2024 •

edited

Loading

rhansen commented Sep 29, 2024

bedevere-app bot commented Sep 29, 2024

bedevere-app bot commented Sep 30, 2024

bedevere-app bot commented Sep 30, 2024

bedevere-app bot commented Sep 30, 2024

bedevere-app bot commented Oct 1, 2024

rhansen commented Oct 1, 2024

1st1 commented Oct 9, 2024

fried commented Oct 10, 2024 •

edited by 1st1

Loading

1st1 commented Oct 12, 2024

pablogsal commented Oct 13, 2024

iritkatriel Oct 13, 2024

rhansen Oct 13, 2024

iritkatriel Oct 13, 2024

rhansen Oct 13, 2024

rhansen Oct 14, 2024

1st1 Oct 14, 2024 •

edited

Loading

1st1 Oct 14, 2024

rhansen Nov 11, 2024

iritkatriel Nov 11, 2024

1st1 Nov 14, 2024

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

Are you sure you want to change the base?

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

Conversation

rhansen commented Sep 29, 2024 • edited Loading

bedevere-app bot commented Sep 29, 2024

rruuaanng commented Sep 29, 2024 • edited Loading

rhansen commented Sep 29, 2024

bedevere-app bot commented Sep 29, 2024

bedevere-app bot commented Sep 30, 2024

bedevere-app bot commented Sep 30, 2024

bedevere-app bot commented Sep 30, 2024

bedevere-app bot commented Oct 1, 2024

rhansen commented Oct 1, 2024

1st1 commented Oct 9, 2024

fried commented Oct 10, 2024 • edited by 1st1 Loading

1st1 commented Oct 12, 2024

pablogsal commented Oct 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1st1 Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhansen commented Sep 29, 2024 •

edited

Loading

rruuaanng commented Sep 29, 2024 •

edited

Loading

fried commented Oct 10, 2024 •

edited by 1st1

Loading

1st1 Oct 14, 2024 •

edited

Loading