8000 bpo-38152: Fix refleak in the finalizer of AST type by eduardo-elizondo · Pull Request #16127 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-38152: Fix refleak in the finalizer of AST type < 8000 span class="f1-light color-fg-muted">#16127

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 14, 2019
Merged

bpo-38152: Fix refleak in the finalizer of AST type #16127

merged 1 commit into from
Sep 14, 2019

Conversation

eduardo-elizondo
Copy link
Contributor
@eduardo-elizondo eduardo-elizondo commented Sep 13, 2019 •
edited by bedevere-bot
Loading

The AST_object was leaking its reference to the AST type. This change properly decreases the reference to the type at deallocation time. Also, this modifies tp_free to use PyType_GetSlot to make PyTypeObject opaque.

https://bugs.python.org/issue38152

@eduardo-elizondo eduardo-elizondo changed the title bpo-352388: Fix refleak in the finalizer of Python-ast.c bpo-352388: Fix refleak in the finalizer of AST type Sep 13, 2019
@eduardo-elizondo eduardo-elizondo changed the title bpo-352388: Fix refleak in the finalizer of AST type bpo-38152: Fix refleak in the finalizer of AST type Sep 13, 2019
@eduardo-elizondo
Copy link
Contributor Author

Previously this was failing, and it's passing now:

eelizondo@build -> ./python -m test test_builtin -R 3:3
Run tests sequentially
0:00:00 load avg: 4.02 [1/1] test_builtin
beginning 6 repetitions
123456
......

== Tests result: SUCCESS ==

1 test OK.

Total duration: 1 sec 339 ms
Tests result: SUCCESS

I also ran all the modules that were referred to by the issue and all of them are passing now:

eelizondo@build -> ./python -m test test_ast test_builtin test_capi test_check_c_globals test_clinic test_compile test_dataclasses test_dbm test_dbm_dumb test_decimal test_fstring test_getpass test_idle test_importlib test_inspect test_pydoc test_source_encoding test_symtable test_sys test_type_comments test_types test_unittest -R 3:3 -j0
Run tests in parallel using 50 child processes
0:00:00 load avg: 8.51 [ 1/22] test_idle skipped
test_idle skipped -- No module named '_tkinter'
0:00:00 load avg: 8.51 [ 2/22] test_symtable passed
beginning 6 repetitions
123456
......
0:00:00 load avg: 8.51 [ 3/22] test_type_comments passed
beginning 6 repetitions
123456
......
0:00:00 load avg: 8.51 [ 4/22] test_check_c_globals passed
beginning 6 repetitions
123456
......
0:00:00 load avg: 8.51 [ 5/22] test_types passed
beginning 6 repetitions
123456
......
0:00:01 load avg: 8.51 [ 6/22] test_getpass passed
beginning 6 repetitions
123456
......
0:00:01 load avg: 8.51 [ 7/22] test_clinic passed
beginning 6 repetitions
123456
......
0:00:02 load avg: 8.51 [ 8/22] test_builtin passed
beginning 6 repetitions
123456
......
0:00:03 load avg: 8.51 [ 9/22] test_dataclasses passed
beginning 6 repetitions
123456
......
0:00:03 load avg: 8.51 [10/22] test_dbm passed
beginning 6 repetitions
123456
......
0:00:04 load avg: 8.51 [11/22] test_source_encoding passed
beginning 6 repetitions
123456
......
0:00:09 load avg: 9.19 [12/22] test_fstring passed
beginning 6 repetitions
123456
......
0:00:09 load avg: 9.19 [13/22] test_dbm_dumb passed
beginning 6 repetitions
123456
......
0:00:10 load avg: 9.65 [14/22] test_inspect passed
beginning 6 repetitions
123456
......
0:00:20 load avg: 10.01 [15/22] test_ast passed
beginning 6 repetitions
123456
......
0:00:21 load avg: 10.01 [16/22] test_compile passed
beginning 6 repetitions
123456
......
0:00:26 load avg: 9.85 [17/22] test_sys passed
beginning 6 repetitions
123456
......
0:00:28 load avg: 9.85 [18/22] test_pydoc passed
beginning 6 repetitions
123456
......
0:00:35 load avg: 9.10 [19/22] test_unittest passed (34 sec 759 ms) -- running: test_capi (35 sec 203 ms), test_decimal (35 sec 196 ms), test_importlib (35 sec 180 ms)
beginning 6 repetitions
123456
......
0:00:42 load avg: 8.61 [20/22] test_importlib passed (42 sec 79 ms) -- running: test_capi (42 sec 574 ms), test_decimal (42 sec 567 ms)
beginning 6 repetitions
123456
......
0:00:45 load avg: 8.56 [21/22] test_decimal passed (45 sec 291 ms) -- running: test_capi (45 sec 730 ms)
beginning 6 repetitions
123456
......
running: test_capi (1 min 15 sec)
running: test_capi (1 min 45 sec)
running: test_capi (2 min 15 sec)
running: test_capi (2 min 45 sec)
running: test_capi (3 min 15 sec)
0:03:44 load avg: 3.57 [22/22] test_capi passed (3 min 44 sec)
beginning 6 repetitions
123456
......

== Tests result: SUCCESS ==

21 tests OK.

1 test skipped:
    test_idle

Total duration: 3 min 44 sec
Tests result: SUCCESS

@eduardo-elizondo
Copy link
Contributor Author

cc @matrixise

ammaraskar
ammaraskar approved these changes Sep 14, 2019
View reviewed changes
Copy link
Member
@ammaraskar ammaraskar left a comment

Choose a reason for hiding this comment

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

LGTM with requested change.

I think a little bit of documentation change would be helpful here as well. With heap-allocated types like this, if there is a custom tp_dealloc then the user must remember to DECREF the type object.

I noticed when you introduced INCREF for heap types, you added a whatsnew entry. I think it would be advisable to add this information to https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_dealloc as well, just so there's somewhere to point to about this behavior.

I would even suggest add a comment here explaining the reason for the DECREF on the type.

Parser/asdl_c.py
@@ -638,9 +638,13 @@ def visitModule(self, mod):
ast_dealloc(AST_object *self)
Copy link
Member

Choose a reason for hiding this comment

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

While you're in here, could you fix the typo introduced here:

cpython/Python/Python-ast.c

Lines 1236 to 1237 in d057b89

{Py_tp_free, PyType_GenericNew},
{Py_tp_free, PyObject_GC_Del},

(Py_tp_free is duplicated, once incorrectly set to PyType_GenericNew)

@DinoV DinoV merged commit 0247e80 into python:master Sep 14, 2019
@bedevere-bot
Copy link

@DinoV: Please replace # with GH- in the commit message next time. Thanks!

@eduardo-elizondo
Copy link
Contributor Author

@ammaraskar makes sense. I'll send a PR updating the docs

@ammaraskar ammaraskar mentioned this pull request Sep 18, 2019
Merged
miss-islington pushed a commit that referenced this pull request Sep 27, 2019
@ammaraskar @miss-islington
bpo-38206: Clarify tp_dealloc requirements for heap allocated types. (G…
5faff97 
…H-16248)

As mentioned in the bpo ticket, this mistake came up on two reviews:
- #16127 (review)
- #16071 (review)

Would be nice to have it documented in a more permanent place than 3.8's whatsnew entry.


https://bugs.python.org/issue38206



Automerge-Triggered-By: @encukou
jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
@ammaraskar
bpo-38206: Clarify tp_dealloc requirements for heap allocated types. (p…
77330ed 
…ythonGH-16248)

As mentioned in the bpo ticket, this mistake came up on two reviews:
- python#16127 (review)
- python#16071 (review)

Would be nice to have it documented in a more permanent place than 3.8's whatsnew entry.


https://bugs.python.org/issue38206



Automerge-Triggered-By: @encukou
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DinoV DinoV DinoV approved these changes

@ammaraskar ammaraskar ammaraskar approved these changes

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@eduardo-elizondo @bedevere-bot @DinoV @ammaraskar @the-knights-who-say-ni
0