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

[Bugfix][FFI] Typo fix of IncRef to DecRef #16021

Merged
merged 2 commits into from
Nov 6, 2023

Conversation

Lunderberg
Copy link
Contributor

Propagation of Python exceptions across C++ stack frames was introduced in #15596. This commit primarily fixes a typo in the initial implementation, where Py_IncRef was used instead of Py_DecRef.

In addition, this PR resolves errors that were exposed by this typo fix, which caused test failures in tests/python/unittest/test_crt.py::test_compile_runtime. These were due to use of the Py_IncRef and Py_DecRef functions on threads that hadn't acquired the GIL. This usage error has been corrected for both the ctypes and cython FFI handling.

Propagation of Python exceptions across C++ stack frames was
introduced in apache#15596.  This commit
primarily fixes a typo in the initial implementation, where
`Py_IncRef` was used instead of `Py_DecRef`.

In addition, this PR resolves errors that were exposed by this typo
fix, which caused test failures in
`tests/python/unittest/test_crt.py::test_compile_runtime`.  These were
due to use of the `Py_IncRef` and `Py_DecRef` functions on threads
that hadn't acquired the GIL.  This usage error has been corrected for
both the ctypes and cython FFI handling.
Copy link
Member

@junrushao junrushao left a comment

Choose a reason for hiding this comment

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

LGTM!

@junrushao junrushao merged commit ffa0033 into apache:main Nov 6, 2023
18 checks passed
@Lunderberg Lunderberg deleted the ffi_incref_decref_typo_fix branch November 6, 2023 14:30
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.

2 participants