Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: main
Are you sure you want to change the base?
gh-124872: Back up exception before calling PyContext_WatchCallback #124741
Changes from 3 commits
c5789a9
4c115ab
300f2fc
f177ad3
f6562b1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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
?There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 toNULL
. So if_PyErr_Occurred
returns anything it must be coming from the callback.There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.