| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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
| 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 |
|---|---|---|
| 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) { | ||
|
Comment thread
Copy link
Copy Markdown
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. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityCan this use cb's return value as before, rather than checking _PyErr_Occurred?
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Contributor
Author
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. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThis 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.
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
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. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityI noticed, but I'm suggesting to not do that. Also, the documentation is incorrect: "If the callback raises an exception it will be ignored."
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Contributor
Author
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. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityAh, 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.)
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Contributor
Author
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. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityI 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.
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
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. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityBut 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.
Sorry, something went wrong.
iritkatriel and ambv reacted with thumbs up emoji
All reactions
Copy link
Copy Markdown
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. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality(FWIW I don't feel strong about this, but I do like the idea somewhat)
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Contributor
Author
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. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality@iritkatriel Does the thumbs-up reaction on @1st1's comment mean you're OK with this PR?
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
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. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityIt 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.
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
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. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality@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.
Sorry, something went wrong.
All reactions
|
||
| PyErr_FormatUnraisable( | ||
| "Exception ignored in %s watcher callback for %R", | ||
| context_event_name(event), ctx); | ||
| } | ||
| _PyErr_SetRaisedException(ts, exc); | ||
| } | ||
| i++; | ||
| bits >>= 1; | ||
| Expand Down | ||
| Back | FazBrowse Home | New Git URL |
Uh oh!
There was an error while loading. Please reload this page.