GH-50591: [Python] Fix reference in ConvertToSequenceAndInferSize when an iterator raises on array conversion - #50594
Conversation
…ze when an iterator raises on array conversion
|
|
|
@github-actions crossbow submit -g python |
|
Revision: afc8c10 Submitted crossbow builds: ursacomputing/crossbow @ actions-1dcc69b782 |
There was a problem hiding this comment.
Pull request overview
Fixes a Python reference leak in ConvertToSequenceAndInferSize when converting a sized iterator that raises during iteration (GH-50591). The change improves exception-safety in the C++/Python bridge by using RAII (OwnedRef) so early returns on Python exceptions don’t skip required decrefs.
Changes:
- Wrap the intermediate
PyList_New(n)result in anOwnedRefanddetach()it only on success to avoid leaking the list/items on exception paths. - Remove manual
Py_DECREF(lst)in the slice-shrinking error path since RAII now handles cleanup. - Add a regression test that asserts refcounts don’t change when an iterator raises during
pa.array(..., size=...).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| python/pyarrow/src/arrow/python/python_to_arrow.cc | Make sized-iterator conversion exception-safe by managing the temporary list with OwnedRef and detaching only on success. |
| python/pyarrow/tests/test_convert_builtin.py | Add a regression test ensuring no reference leak when a raising iterator is converted with size specified. |
|
@pitrou I think this is another reference leak |
| // either an error occurred or the iterator ended | ||
| RETURN_IF_PYERROR(); |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 826ac37. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Rationale for this change
We currently leak the list and items in the case of an iterator raising an Exception and size is present.
What changes are included in this PR?
Use
OwnedRefinstead of relying on manualPy_DECREFin order to avoid exception scenario returning early inRETURN_IF_PYERROR.Are these changes tested?
Yes, I've created a new test that fails previously to the fix and passes after the fix
Are there any user-facing changes?
No