bpo-44353: Fix memory leak introduced by #27262#27305
bpo-44353: Fix memory leak introduced by #27262#27305ambv merged 3 commits intopython:mainpython/cpython:mainfrom uriyyo:fix-issue-44353-memory-leakuriyyo/cpython:fix-issue-44353-memory-leakCopy head branch name to clipboard
Conversation
|
@pablogsal This should fix memory leak introduced by #27262 And could you please add |
pablogsal
left a comment
There was a problem hiding this comment.
This is not correct, you are returning a new object without incrementing the refcount, this will crash surely
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
@uriyyo, look at #27262 (review) |
|
Hmm, I will try to investigate where memory leak is. |
|
@ambv Thanks for comment. Did forget about it. |
|
@pablogsal @ambv I have found were the memory leak is, that's not because of C extension, sorry about confusing with me first comment. Memory leak was because of inner This lines lead to memory leak: cpython/Lib/test/test_typing.py Lines 3723 to 3727 in a4760cc I fixed this issue by clearing cache after test execution. |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @pablogsal: please review the changes made to this pull request. |
ambv
left a comment
There was a problem hiding this comment.
Confirmed locally this fixes the refleaks.
Without the change:
❯ ./python.exe -m test test_typing -R :
+ cd cpython
+ shift
+ PYTHONWARNINGS=
+ PYTHONSTARTUP=
+ IN_REGRTEST=1
+ ./python.exe -E -Wd -m test test_typing -R :
0:00:00 load avg: 1.99 Run tests sequentially
0:00:00 load avg: 1.99 [1/1] test_typing
beginning 9 repetitions
123456789
.........
test_typing leaked [220, 220, 220, 220] references, sum=880
test_typing leaked [68, 68, 68, 68] memory blocks, sum=272
test_typing failed (reference leak)
== Tests result: FAILURE ==
1 test failed:
test_typing
1 re-run test:
test_typing
Total duration: 5.6 sec
Tests result: FAILURE
After the change:
❯ ./python.exe -m test test_typing -R :
+ cd cpython
+ shift
+ PYTHONWARNINGS=
+ PYTHONSTARTUP=
+ IN_REGRTEST=1
+ ./python.exe -E -Wd -m test test_typing -R :
0:00:00 load avg: 2.03 Run tests sequentially
0:00:00 load avg: 2.03 [1/1] test_typing
beginning 9 repetitions
123456789
.........
== Tests result: SUCCESS ==
1 test OK.
Total duration: 5.5 sec
Tests result: SUCCESS
|
|
||
|
|
||
| class NewTypeTests: | ||
| def cleanup(self): |
There was a problem hiding this comment.
Note to self: this is different from BaseTestCase.clear_caches because here we use self.module instead of hard-coded typing.
There was a problem hiding this comment.
Wow, great investigating @uriyyo! I forgot that the global typing and the py_typing used for NewType tests are different module objects. I was quite puzzled for a while about why the BaseTestCase clearing didn't work :).
There was a problem hiding this comment.
@Fidget-Spinner I spent a lot of time investigating C code because though that problem was at it, but actual problem was at test code)
It was tricky but interesting to investigate this memory leak)
| class NewTypeTests: | ||
| def cleanup(self): | ||
| for f in self.module._cleanups: | ||
| f() |
There was a problem hiding this comment.
Those seem to be cleaning LRU caches for various type __getitem__.
There was a problem hiding this comment.
Yup, and it clears Union.__getitem__ cache which lead to memory leak.
https://bugs.python.org/issue44353