Skip to content

GH-106176, GH-104702: Fix reference leak when importing across multiple threads #108497

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
vstinner merged 6 commits into from
Aug 29, 2023
Merged

GH-106176, GH-104702: Fix reference leak when importing across multiple threads #108497

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
50e4c5d
GH-106176: Fix reference leak when importing across multiple threads
brettcannon Aug 25, 2023
a2e94c3
Address reviewer comments
brettcannon Aug 28, 2023
7d90811
Merge branch 'main' into issue-106176-_blocking_on-WeakValueDictionary
brettcannon Aug 28, 2023
432bea4
Make a name very clearly private
brettcannon Aug 29, 2023
c841af7
Merge branch 'main' into issue-106176-_blocking_on-WeakValueDictionary
brettcannon Aug 29, 2023
78f5f7a
Merge branch 'main' into issue-106176-_blocking_on-WeakValueDictionary
brettcannon Aug 29, 2023
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
Prev Previous commit
Next Next commit
Make a name very clearly private
  • Loading branch information
@brettcannon
brettcannon committed Aug 29, 2023
commit 432bea421e0dd5c4672d94fca5851f70c4325bec
4 changes: 2 additions & 2 deletions Lib/importlib/_bootstrap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ def _new_module(name):
# Module-level locking ########################################################

# For a list that can have a weakref to it.
class List(list):
class _List(list):
pass
Copy link
Member

@Yhg1s Yhg1s Aug 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

FWIW, I think it would be nice to use __slots__ for this class, to reduce the size of each instance (especially since the setdefault call creates one each time, even when it isn't necessary).

Copy link
Member Author

@brettcannon brettcannon Aug 29, 2023

Choose a reason for hiding this comment

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

As in you're going to make a PR to make that change, or you're hoping someone else will open an issue on your behalf to track the idea?

Copy link
Member

@Yhg1s Yhg1s Sep 18, 2023

Choose a reason for hiding this comment

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

The latter (although not on my behalf) -- I don't know if there's a reason not to do it, and I don't have the spare cycles to dig in and find out.



Expand Down Expand Up @@ -167,7 +167,7 @@ def __enter__(self):
# re-entrant (i.e., a single thread may take it more than once) so it
# wouldn't help us be correct in the face of re-entrancy either.

self.blocked_on = _blocking_on.setdefault(self.thread_id, List())
self.blocked_on = _blocking_on.setdefault(self.thread_id, _List())
self.blocked_on.append(self.lock)

def __exit__(self, *args, **kwargs):
Expand Down