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

fix: properly translate C++ exception to Python exception when creating Python buffer from wrapped object #5324

Merged
merged 9 commits into from
Sep 2, 2024

Conversation

vnlitvinov
Copy link
Contributor

@vnlitvinov vnlitvinov commented Aug 21, 2024

Description

  • Refactor exception translation a little - extract inline translation to a callable helper;
  • Call the helper if an exception occurs when trying to get a buffer from wrapped object;
  • Added the test, which crashes with Fatal Python error without the fix (unhandlable from within Python) and passes after it is done.

Closes: #2764, #3336

Suggested changelog entry:

* Properly translate C++ exception to Python exception when creating Python buffer from wrapped object

@rwgk
Copy link
Collaborator

rwgk commented Aug 23, 2024

I need to find a block of time to look carefully.

But high-level: There is already too much stuff in internals.h, adding in the exception translator code seems very unfortunate.

pybind11 is poorly factored in general, I'd love to not make it worse, even though it can get tricky sometimes.

Could you please try to move the exception translator code into its own header, maybe pybind11/detail/exception_translators.h?

You'll have to update a couple files when adding a new header. An easy way to pin-point the locations is e.g.:

$ git grep detail/value_and_holder.h
CMakeLists.txt:    include/pybind11/detail/value_and_holder.h
tests/extra_python_package/test_files.py:    "include/pybind11/detail/value_and_holder.h",

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

A couple minor suggestions. I only glanced through. Looks great, although I still want to look more carefully later (maybe weekend).

include/pybind11/detail/class.h Show resolved Hide resolved
tests/test_buffers.cpp Outdated Show resolved Hide resolved
tests/test_buffers.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

The suggestions are really minor.

include/pybind11/detail/class.h Show resolved Hide resolved
include/pybind11/detail/exception_translation.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

@henryiii @EthanSteinberg @Skylion007: Do you have ~10 minutes for a second review?

Copy link
Collaborator

@EthanSteinberg EthanSteinberg left a comment

Choose a reason for hiding this comment

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

This looks good to me, nice job!

(As a side note though, def_buffer seems to have a way more convoluted design than it should ...)

@rwgk
Copy link
Collaborator

rwgk commented Sep 2, 2024

Thanks a lot @EthanSteinberg for the review!

@rwgk rwgk merged commit aeda49e into pybind:master Sep 2, 2024
78 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Sep 2, 2024
@henryiii henryiii changed the title Properly translate C++ exception to Python exception when creating Python buffer from wrapped object fix: properly translate C++ exception to Python exception when creating Python buffer from wrapped object Sep 13, 2024
henryiii pushed a commit that referenced this pull request Sep 13, 2024
…thon buffer from wrapped object (#5324)

* Add test for throwing def_buffer case

* Translate C++ -> Python exceptions in buffer creation

This required a little refactoring to extract exception translation to a separate method

* Fix code formatting

* Fix "unused parameter" warning

* Refactor per review

* style: pre-commit fixes

* Address review comments

* Address review comments

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
henryiii pushed a commit that referenced this pull request Sep 13, 2024
…thon buffer from wrapped object (#5324)

* Add test for throwing def_buffer case

* Translate C++ -> Python exceptions in buffer creation

This required a little refactoring to extract exception translation to a separate method

* Fix code formatting

* Fix "unused parameter" warning

* Refactor per review

* style: pre-commit fixes

* Address review comments

* Address review comments

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs changelog Possibly needs a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]throw in def_buffer cause terminate
3 participants