GeezerVison

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

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.

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.

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

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

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

(Summary: refleaks in extension module tend to hold typed function signatures alive for eternity. This function signature references typing.py internals (LRU caches), which in turn cast a web of references to other parts of the interpreter)

In this way, the caching mechanism in typing.py can turn virtually any refleak in any typed extension module into refleaks elsewhere. Ultimately, it becomes difficult to develop leak-free extensions, because any systematic checking for leaks runs into flukes. I'm creating a framework for extension modules that tries to be well-behaved in this ecosystem, but its internal tests for leaks are hampered by this behavior. Quick 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.

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

If by "the tool", you mean the nanobind-based reproducer, please let me know. It would be straightforward to reproduce the leak with a minimal CPython extension.

I can sit down and make such a reproducer, but I would prefer to only do so if it's time well spent (in the sense that there is willingness to fix the issue if convincingly demonstrated).

Speaking as a typing maintainer,

Theoretically, I would like to say that the onus is on the external code to fix their refleaks. Since in this case as you have pointed out, typing.py doesn't have any refleaks. It just worsens external ones.

However, I'm not opposed to the change (+0.5), because there aren't that many more lines of code. So the maintenance burden to support something like this isn't large.

Also as you have rightly pointed out, it's nearly impossible to get all extension modules to fix all their refleaks. So I'm satisfied with typing not making them worse.

@wjakob Never mind. I trust @Fidget-Spinner here.

static global state in extension modules is bad and should be avoided as much. See https://peps.python.org/pep-0489/
They are bad for multiple reasons not to mention they are a problem for sub-interpreter. I worry if we add workarounds for this in stdlib then library authors will never fix their own code and we will get more requests to add workarounds so that they don't have to fix their own code. Note that just clearing reference isn't enough here, when the GC is involved this can lead to very strange bugs and are a pain to debug. See a similar asyncio bug #96232 involving static c globals. For this specific case, this seems fine but we need to be cautious because if we add this then the extension authors will never notice that there is a bug in their code as it will probably "just work".

P.S I am not a typing maintainer so I'll defer to @Fidget-Spinner.

There are a few misconceptions in the above message, I'll try to clarify.

static global state in extension modules is bad and should be avoided as much. [..] I worry if we add workarounds for this in stdlib then library authors will never fix their own code and we will get more requests to add workarounds so that they don't have to fix their own code.

While this PR does introduce a static state variable, it represents state that is already static (the caches). FWIW there is a _cleanup variable just a few lines above that serves the exact same purpose, so it does not seem like new ground is being broken.

[..] if we add this then the extension authors will never notice that there is a bug in their code as it will probably "just work".

This is already what is happening. The leaks in question don't cause issues for those respective packages.

The problem is that the internal mechanics of the typing module spread these leaks to other modules, which is where it becomes problematic. As mentioned in the associated issue, expecting all other extensions to be fixed is is going to be an impossible game of whack-a-mole.

While this PR does introduce a static state variable, it represents state that is already static (the caches). FWIW there is a _cleanup variable just a few lines above that serves the exact same purpose, so it does not seem like new ground is being broken.

_cleanups has a different purpose, it is used by our test runner to check for reference leaks by clearing caches and it not used outside of that purpose.

While this PR does introduce a static state variable

There seems to be a miscommunication here. When Kumar wrote "static global state in extension modules is bad" he meant C code (maybe referring to the bug you found in some extension?). This has nothing to do with static state in a .py module like referenced by this PR. So let's please all calm down.

@rhettinger : I added a longer paragraph summarizing the issue and incorporated your feedback.

Please don't force-push to CPython PRs after review/discussion has started. It interacts badly with GitHub's UI, making it harder to see what exactly changed since we last looked at the PR :)

Sorry 😇. For posterity, I amended the commit to replace the contents of the file Misc/NEWS.d/next/Library/2022-10-24-11-01-05.gh-issue-98253.HVd5v4.rst.

Dear all,

It seems to me that the decision process regarding this PR has stalled. It would be good to come to a decision on how to proceed (accept, reject, further changes requested, ...?)

Thanks in advance!
Wenzel

I will accept it as an enhancement to typing soon (ie it wont be backported). Sorry for the wait, I was busy with uni exams.

@serhiy-storchaka or @JelleZijlstra does the code look okay to y'all? I plan to merge this soon.

It's quite puzzling -- I pushed two further commits to address your feedback, but they don't show up here. Perhaps a GitHub service issue/outage? See main...wjakob:cpython:typing-leak

It's quite puzzling -- I pushed two further commits to address your feedback, but they don't show up here. Perhaps a GitHub service issue/outage? See main...wjakob:cpython:typing-leak

GitHub is quite laggy right now in general, I'm seeing it on typeshed and typing_extensions as well :)

Yes, GitHub is apparently having some issues; I saw it on other PRs. I did get emails for the two commits you pushed.

Edit: now, something is happening.

https://www.githubstatus.com/ reports some degradation.