Skip to content
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

Add wrapper for PyErr_CheckSignals() to Python. #1214

Merged
merged 1 commit into from
Oct 8, 2020

Conversation

birkenfeld
Copy link
Member

I don't know how to test this...

@kngwyu
Copy link
Member

kngwyu commented Oct 8, 2020

Looks good but we have to fix PyPy CI first (it's unrelated to this patch, though).

@davidhewitt
Copy link
Member

CI actually only prevents merging if any of the python 3.8 jobs fail atm, so no need to rebase on #1215.

I was going to ask though - should this api be PyErr::check_signals instead of Python::check_signals? We already also have PyErr_WarnEx wrapped on PyErr in this way.

@birkenfeld
Copy link
Member Author

birkenfeld commented Oct 8, 2020

Well IMO this only has a marginal connection to PyErr and exception handling, and the C version is named unfortunately. It's rather an interpreter related thing.

In newer Pythons, they added APIs with PySignal_ prefix but this didn't exist back then.

@davidhewitt
Copy link
Member

👍

I think it's ok to let a test slide on this one for now as it's a complicated test to write, and this is basically just wrapping the C-API?

Could use a CHANGELOG entry stating the addition of the new method.

Comment on lines +502 to +507
/// Lets the Python interpreter check for pending signals and invoke the
/// corresponding signal handlers. This can run arbitrary Python code.
///
/// If an exception is raised by the signal handler, or the default signal
/// handler raises an exception (such as `KeyboardInterrupt` for `SIGINT`),
/// an `Err` is returned.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this doc maybe also mention that it's a safe wrapper about PyErr_CheckSignals? I think it can be helpful for some people to know this without having to look at the source.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

This is a useful API in long-running Rust code, which lets users
cancel evaluation by pressing Ctrl-C, and run any other signal
handlers that have been set using the signal module.
Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@kngwyu kngwyu merged commit 1d34ed7 into PyO3:master Oct 8, 2020
@birkenfeld birkenfeld deleted the signals branch May 4, 2021 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants