Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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
ambv 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 9 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):

@ambv ambv Jul 23, 2021

Copy link
Copy Markdown
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.

@Fidget-Spinner Fidget-Spinner Jul 23, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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

@uriyyo uriyyo Jul 23, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

@ambv ambv Jul 23, 2021

Copy link
Copy Markdown
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__.

@uriyyo uriyyo Jul 23, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.