Skip to content

fix: better exception and error handling for capsules#3825

Merged
Skylion007 merged 2 commits into
pybind:masterfrom
Skylion007:skylion007/fix-capsule-errors
Mar 25, 2022
Merged

fix: better exception and error handling for capsules#3825
Skylion007 merged 2 commits into
pybind:masterfrom
Skylion007:skylion007/fix-capsule-errors

Conversation

@Skylion007

@Skylion007 Skylion007 commented Mar 24, 2022

Copy link
Copy Markdown
Collaborator

Description

  • This solves several issues when dealing with capsules. Firstly, we were leaving the pybind11 in a bad exception state when we throw pybind11_fail without clearing the error first. We really should be throwing error_already_set to ensure that exception messages are as consistent and informative as vanilla cpython.
  • There really isn't a good reason to be catching these errors so the changes should be pretty backwards compatible. In fact, a lot of the invalid error states occurred because our test suite never hit them in production.
  • This guards against some nasty segfaults that could occur when mishandling capsules.
  • Many of the python capsule calls only return null on error states which makes exception handling trivial. There was only one case where I had to add an additional pybind11_fail as a catch.
  • Finally while browsing the repo, I found one more missing std::move and added that in.

Suggested changelog entry:

* The bindings for capsules now have more consistent exception handling.

@Skylion007
Skylion007 requested review from henryiii and rwgk March 24, 2022 19:21

@rwgk rwgk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks a lot for this cleanup, the revised code makes a lot more sense!
I'll try to run this through our global testing tonight, but if you need this before I have the results, please go ahead.

@rwgk

rwgk commented Mar 24, 2022

Copy link
Copy Markdown
Collaborator

Suggested changelogentry:
PyCapsules standing bindings now have more consistent exception handling.

standing?

standard?

Maybe: py::capsule now has more consistent exception handling.

?

@rwgk

rwgk commented Mar 25, 2022

Copy link
Copy Markdown
Collaborator

The test ID for the global testing is OCL:437135289:BASE:437101826:1648173046939:234ba7aa, 21:00 batch.

@rwgk

rwgk commented Mar 25, 2022

Copy link
Copy Markdown
Collaborator

The global testing finished without finding any issues.

@Skylion007

Copy link
Copy Markdown
Collaborator Author

@rwgk Fixed spelling errors in the PR description.

@henryiii henryiii left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks fantastic to me. :)

@Skylion007 Skylion007 changed the title bugfix: better exception and error handling for capsules fix: better exception and error handling for capsules Mar 25, 2022
@Skylion007
Skylion007 merged commit 146695a into pybind:master Mar 25, 2022
@Skylion007
Skylion007 deleted the skylion007/fix-capsule-errors branch March 25, 2022 14:55
@github-actions github-actions Bot added the needs changelog Possibly needs a changelog entry label Mar 25, 2022
henryiii pushed a commit to henryiii/pybind11 that referenced this pull request Mar 25, 2022
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Mar 29, 2022
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.

3 participants