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.

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

Merged
merged 3 commits into from
Jul 23, 2021
Merged

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d6ff295
bpo-44353: Fix memory leak at _typing__idfunc
uriyyo Jul 23, 2021
c4dfbcf
Clear typing module cache after test execution
uriyyo Jul 23, 2021
c27c643
Set correct base cls order
uriyyo Jul 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions Lib/test/test_typing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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)

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 •
edited
Loading

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.


def setUp(self):
sys.modules['typing'] = self.module

def tearDown(self):
self.cleanup()
sys.modules['typing'] = typing

def test_basic(self):
Expand Down Expand Up @@ -3738,12 +3743,12 @@ def test_repr(self):

self.assertEqual(repr(UserId), f'{__name__}.UserId')

class NewTypePythonTests(BaseTestCase, NewTypeTests):
class NewTypePythonTests(NewTypeTests, BaseTestCase):
module = py_typing


@skipUnless(c_typing, 'requires _typing')
class NewTypeCTests(BaseTestCase, NewTypeTests):
class NewTypeCTests(NewTypeTests, BaseTestCase):
module = c_typing


Expand Down