Skip to content

bpo-44353: Fix memory leak introduced by #27262 #27305

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

Merged
merged 3 commits into from
Jul 23, 2021

Conversation

uriyyo
Copy link
Member

@uriyyo uriyyo commented Jul 23, 2021

@uriyyo
Copy link
Member Author

uriyyo commented Jul 23, 2021

@pablogsal This should fix memory leak introduced by #27262

And could you please add skip-news label?

Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

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

This is not correct, you are returning a new object without incrementing the refcount, this will crash surely

@bedevere-bot
Copy link

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@ambv
Copy link
Contributor

ambv commented Jul 23, 2021

@uriyyo, look at #27262 (review)

@uriyyo
Copy link
Member Author

uriyyo commented Jul 23, 2021

Hmm, I will try to investigate where memory leak is.

@uriyyo
Copy link
Member Author

uriyyo commented Jul 23, 2021

@ambv Thanks for comment. Did forget about it.

@uriyyo uriyyo changed the title bpo-44353: Fix memory leak at _typing__idfunc bpo-44353: Fix memory leak introduced by #27262 Jul 23, 2021
@uriyyo
Copy link
Member Author

uriyyo commented Jul 23, 2021

@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 typing module cache when typing.Union was created.

This lines lead to memory leak:

self.assertEqual(UserId | cls, self.module.Union[UserId, cls])
self.assertEqual(cls | UserId, self.module.Union[cls, UserId])
self.assertEqual(self.module.get_args(UserId | cls), (UserId, cls))
self.assertEqual(self.module.get_args(cls | UserId), (cls, UserId))

I fixed this issue by clearing cache after test execution.

@uriyyo
Copy link
Member Author

uriyyo commented Jul 23, 2021

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@pablogsal: please review the changes made to this pull request.

Copy link
Contributor

@ambv ambv left a comment

Choose a reason for hiding this comment

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

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

@@ -3692,10 +3692,15 @@ def test_c_functions(self):


class NewTypeTests:
def cleanup(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self: this is different from BaseTestCase.clear_caches because here we use self.module instead of hard-coded typing.

Copy link
Member

@Fidget-Spinner Fidget-Spinner Jul 23, 2021

Choose a reason for hiding this comment

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

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 :).

Copy link
Member Author

@uriyyo uriyyo Jul 23, 2021

Choose a reason for hiding this comment

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

@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)

@@ -3692,10 +3692,15 @@ def test_c_functions(self):


class NewTypeTests:
def cleanup(self):
for f in self.module._cleanups:
f()
Copy link
Contributor

Choose a reason for hiding this comment

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

Those seem to be cleaning LRU caches for various type __getitem__.

Copy link
Member Author

@uriyyo uriyyo Jul 23, 2021

Choose a reason for hiding this comment

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

Yup, and it clears Union.__getitem__ cache which lead to memory leak.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants