Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
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?
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
c5789a94c115ab300f2fcf177ad3f6562b1File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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){ | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this use ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR changes the callback's return type to Member There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 (I should express this rationale in the commit message and PR description.) ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But in this case we know where the exception is coming from, Member There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It related to the comment it's on. I would prefer avoiding Member There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
Uh oh!
There was an error while loading. Please reload this page.