gh-124872: Back up exception before calling PyContext_WatchCallback#124741
Conversation
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 |
I think it still needs to be added because this behavior is new. |
rhansen commented Sep 29, 2024
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? |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
rhansenforce-pushed the gh-119333-exception branch from 1636f10 to 9e4ea98CompareMost 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 |
Uh oh!
There was an error while loading. Please reload this page.
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 |
2 similar comments
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 |
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 |
rhansenforce-pushed the gh-119333-exception branch from 212948c to 5f8590cCompareMost 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 |
rhansen changed the title rhansen commented Oct 1, 2024
Apologies for the force-push; I wanted to update the commit message to reference the new issue I just opened (gh-124872). |
1st1 commented Oct 9, 2024
I'm +0 for this change. Curious what @fried would say. |
The existing behavior mirrors the other object event "watchers" |
1st1 commented Oct 12, 2024
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.
rhansenforce-pushed the gh-119333-exception branch from 5f8590c to ca49b94Compare
pablogsal commented Oct 13, 2024
I'm with @1st1 on this one (+1 to this design). Maybe the C-API working group has some input? @iritkatriel |
| if (cb(event, ctx) <0){ | ||
| PyObject*exc=_PyErr_GetRaisedException(ts); | ||
| cb(event, ctx); | ||
| if (_PyErr_Occurred(ts) !=NULL){ |
There was a problem hiding this comment.
Can this use cb's return value as before, rather than checking _PyErr_Occurred?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(FWIW I don't feel strong about this, but I do like the idea somewhat)
There was a problem hiding this comment.
@iritkatriel Does the thumbs-up reaction on @1st1's comment mean you're OK with this PR?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
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.
rhansenforce-pushed the gh-119333-exception branch from ca49b94 to 300f2fcCompare8 participants
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
inttovoid. 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
📚 Documentation preview 📚: https://cpython-previews--124741.org.readthedocs.build/