-
-
Notifications
You must be signed in to change notification settings - Fork 34.5k
GH-81061: Fix refcount issue when returning None from a ctypes.py_object callback
#13364
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
Changes from 3 commits
85026c5
2a5aeba
611f1b6
fb17c68
66ea175
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,5 +97,31 @@ def func(a, b): | |
| f(1, 2) | ||
| self.assertEqual(sys.getrefcount(ctypes.c_int), a) | ||
|
|
||
| @support.refcount_test | ||
| def test_callback_py_object_none_return(self): | ||
| """Test that returning ``None`` from a ``py_object`` callback | ||
| does not affect ``None``'s refcount (bpo-36880).""" | ||
|
dgelessus marked this conversation as resolved.
Outdated
|
||
|
|
||
| import sys | ||
|
eryksun marked this conversation as resolved.
Outdated
|
||
|
|
||
| for FUNCTYPE in (ctypes.CFUNCTYPE, ctypes.PYFUNCTYPE): | ||
| with self.subTest(FUNCTYPE=FUNCTYPE): | ||
| proto = FUNCTYPE(ctypes.py_object) | ||
| @proto | ||
|
dgelessus marked this conversation as resolved.
Outdated
|
||
| def func(): | ||
| return None | ||
|
|
||
| # Check that calling func does not affect None's refcount. | ||
| none_refcount = sys.getrefcount(None) | ||
| # Because None's refcount can also change for other reasons, | ||
| # we call func in a loop to ensure that any effects on None's | ||
| # refcount are clearly visible. | ||
| for _ in range(10000): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Single call should be enough to trigger the refleak checker.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately not... I tested again - apparently the refleak checker (with default settings:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I see, it is because of #74959. |
||
| func() | ||
| # Allow for small variations in None's refcount from other | ||
| # sources. | ||
| self.assertAlmostEqual( | ||
| sys.getrefcount(None), none_refcount, delta=50) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, in general we don't do these kind of tests. We have an automatic refleak detection mechanism but it will only work if the path that leaks references is exercised in the test suite. This means that you can add a test that ensures that the code you are adding to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah okay. I was going by what the other tests in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not as part of this issue (one issue per problem - only changing what's required for the problem at hand). I haven't checked those and maybe these are checked more specific things.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can confirm that your test work running `./python -m test test_ctypes -R'. Ensure that it detects the problem without your fix first :)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I didn't know that just |
||
|
|
||
| if __name__ == '__main__': | ||
| unittest.main() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Fix a reference counting issue when a :mod:`ctypes` callback with return | ||
| type :class:`~ctypes.py_object` returns ``None``, which could cause crashes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT, this test is rendered pointless in 3.12 due to the adoption of PEP 683 (Immortal Objects, Using a Fixed Refcount). Because
Noneis immortal, executingPy_INCREF(Py_None)andPy_DECREF(Py_None)will no longer modify its refcount.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good to know. I assume this isn't implemented yet though, because as of the current state of
main(447d061), this test still hard-crashes without the fix.