send errors in `releasebuffer` to `sys.unraisablehook` #2886

davidhewitt · 2023-01-18T08:56:28Z

I noticed that in the __releasebuffer__ implementation we currently diverge from documentation by allowing -> PyResult<()> return values, but they cause crashes later down the line.

e.g. without the other changes in this PR, the new test case would crash with the following message:

ValueError: oh dear

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
SystemError: <class 'bytes'> returned a result with an exception set

After some deliberation I decided to allow -> PyResult<()> returns because:

it keeps the macro implementation and usage simpler
errors might be produced by the __releasebuffer__ internals anyway (e.g. due to PyCell locking, or invalid self type passed)

As a result, this PR cleans up the case discussed to send errors to sys.unraisablehook, and updates the documentation to be clearer on the allowed behaviour.

src/impl_/trampoline.rs

src/impl_/pyclass.rs

newsfragments/2886.fixed.md

tests/test_buffer_protocol.rs

davidhewitt · 2023-01-18T20:54:21Z

Thanks as always for the thorough review! I've pushed a commit which should address all of those points and hopefully pass CI.

davidhewitt · 2023-01-19T08:07:31Z

bors r=adamreichold

2886: send errors in `__releasebuffer__` to `sys.unraisablehook` r=adamreichold a=davidhewitt I noticed that in the `__releasebuffer__` implementation we currently diverge from documentation by allowing `-> PyResult<()>` return values, but they cause crashes later down the line. e.g. without the other changes in this PR, the new test case would crash with the following message: ``` ValueError: oh dear The above exception was the direct cause of the following exception: Traceback (most recent call last): File "<string>", line 1, in <module> SystemError: <class 'bytes'> returned a result with an exception set ``` After some deliberation I decided to allow `-> PyResult<()>` returns because: - it keeps the macro implementation and usage simpler - errors might be produced by the `__releasebuffer__` internals anyway (e.g. due to `PyCell` locking, or invalid self type passed) As a result, this PR cleans up the case discussed to send errors to `sys.unraisablehook`, and updates the documentation to be clearer on the allowed behaviour. Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

bors · 2023-01-19T08:34:38Z

Canceled.

davidhewitt · 2023-01-19T08:34:47Z

bors r+

2886: send errors in `__releasebuffer__` to `sys.unraisablehook` r=davidhewitt a=davidhewitt I noticed that in the `__releasebuffer__` implementation we currently diverge from documentation by allowing `-> PyResult<()>` return values, but they cause crashes later down the line. e.g. without the other changes in this PR, the new test case would crash with the following message: ``` ValueError: oh dear The above exception was the direct cause of the following exception: Traceback (most recent call last): File "<string>", line 1, in <module> SystemError: <class 'bytes'> returned a result with an exception set ``` After some deliberation I decided to allow `-> PyResult<()>` returns because: - it keeps the macro implementation and usage simpler - errors might be produced by the `__releasebuffer__` internals anyway (e.g. due to `PyCell` locking, or invalid self type passed) As a result, this PR cleans up the case discussed to send errors to `sys.unraisablehook`, and updates the documentation to be clearer on the allowed behaviour. Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

bors · 2023-01-19T08:56:49Z

Canceled.

davidhewitt · 2023-01-19T08:57:00Z

bors r=adamreichold

2886: send errors in `__releasebuffer__` to `sys.unraisablehook` r=adamreichold a=davidhewitt I noticed that in the `__releasebuffer__` implementation we currently diverge from documentation by allowing `-> PyResult<()>` return values, but they cause crashes later down the line. e.g. without the other changes in this PR, the new test case would crash with the following message: ``` ValueError: oh dear The above exception was the direct cause of the following exception: Traceback (most recent call last): File "<string>", line 1, in <module> SystemError: <class 'bytes'> returned a result with an exception set ``` After some deliberation I decided to allow `-> PyResult<()>` returns because: - it keeps the macro implementation and usage simpler - errors might be produced by the `__releasebuffer__` internals anyway (e.g. due to `PyCell` locking, or invalid self type passed) As a result, this PR cleans up the case discussed to send errors to `sys.unraisablehook`, and updates the documentation to be clearer on the allowed behaviour. Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

bors · 2023-01-19T09:38:59Z

Build failed:

conclusion

davidhewitt · 2023-01-19T19:15:23Z

bors r=adamreichold

bors · 2023-01-19T20:08:54Z

Build succeeded:

conclusion

davidhewitt mentioned this pull request Jan 18, 2023

Expose PyErr_WriteUnraisable #2884

Closed