Skip to content
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

gh-98253: fix refleak issue in typing.py #98591

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wjakob
Copy link
Contributor

@wjakob wjakob commented Oct 24, 2022

The typing module internally uses LRU caches to memoize the result of
type-related computations. These LRU caches have the potential to introduce
difficult-to-find reference leaks spanning multiple extension modules.

Suppose that a binary extension module a leaks a reference to a type a.A
with typed function signatures.

The leaked type a.A will induce a secondary refleak of the typing LRU
caches, which will at this point cause tertiary leaks in entirely unrelated
extension modules. In this way, a benign refleak in any extension can cause
difficult-to-debug leaks everywhere else.

This commit fixes this by removing the direct reference from a.A to
typing implementation details.

@wjakob
Copy link
Contributor Author

wjakob commented Oct 24, 2022

PS: It would be good to merge this not only in Python 3.12, but also older versions (3.11, 3.10, 3.9, 3.8).

@AlexWaygood
Copy link
Member

AlexWaygood commented Oct 24, 2022

PS: It would be good to merge this not only in Python 3.12, but also older versions (3.11, 3.10, 3.9, 3.8).

3.8 and 3.9 are now only accepting security-related patches, so this definitely won't be backported that far, I'm afraid.

@wjakob
Copy link
Contributor Author

wjakob commented Oct 24, 2022

3.8 and 3.9 are now only accepting security-related patches, so this definitely won't be backported that far, I'm afraid.

Fair enough. In that case, I propose to target 3.10, 3.11, and 3.12.

@JelleZijlstra
Copy link
Member

JelleZijlstra commented Oct 24, 2022

As mentioned in the issue I'm skeptical that this is something we should fix in typing.py. I'll ask for input from other core devs (after the dust from the 3.11 release settles).

@gvanrossum
Copy link
Member

gvanrossum commented Oct 24, 2022

I'm skeptical too. I feel that the tool is incorrectly blaming typing.py.

@wjakob
Copy link
Contributor Author

wjakob commented Oct 24, 2022

@gvanrossum, did you see the discussion in #98253? When I first reported the issue, the actual source of these refleaks was mysterious. Further along the discussion thread, the problem was finally tracked down (see post #98253 (comment))

The problem is that the caching mechanism in typing.py can turn virtually any refleak in any typed extension module into refleaks elsewhere. That makes it really difficult to develop leak-free extensions, because any systematic checking for leaks runs into flukes. My tests show that many of the bigger packages (pandas, pytorch, tensorflow) include some refleaks that are problematic in this context.

The patch in this PR is simple and it breaks this problematic chain of references. Of course, one could say: let's leave typing.py as-is and fix all of the other extension modules. I just don't think it is very practical to do so.

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.

None yet

5 participants