Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-116946: Remove gc-support from some immutable types#139073
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-116946: Remove gc-support from some immutable types #139073
Uh oh!
There was an error while loading. Please reload this page.
Conversation
sergey-miryanov commented Sep 17, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
sergey-miryanov commented Sep 17, 2025
As @picnixz suggested this should skip news. |
sergey-miryanov commented Sep 17, 2025
This ready to review, please take a look. |
picnixz commented Sep 17, 2025
I'll have a look tomorrow or later tonight |
neonene commented Sep 17, 2025
Be careful not to create ref cycles. At least, |
picnixz commented Sep 17, 2025
That's news to me! Could you have a look at the types I said "no need" to see if it's really the case? or link the relevant docs (I'm on mobile) |
neonene commented Sep 17, 2025
Heaptype without GC is not recommended/clarified to users for its complexity of holding/freeing the module properly: #118021 (comment) Regarding the module:type cycle, it looks to me like the following: |
sergey-miryanov commented Sep 18, 2025
I'm not sure I get it. IIUC, the Maybe I'm missing something? |
neonene commented Sep 18, 2025
Probably I'm wrong. After reading the PEP-573 again, I realized it says that the module finalization phase breaks the module:type cycle even without the GC. If I'm reading correctly, I think the section still stands. I'll check what I'm missing. |
neonene commented Sep 18, 2025
importgc, bz2_=bz2.BZ2Compressor() gc.collect()It seems like #20960 just forgot to add the GC flag when the isolation howto is not yet established. |
sergey-miryanov commented Sep 18, 2025
Then we should restore GC-support for these types. @picnixz OK? |
sergey-miryanov commented Sep 18, 2025
And maybe check for other too. |
picnixz commented Sep 18, 2025
Let's put this on hold until we all agree about the fact that immutable empty types do not need a priori the GC but that they may require it in some exceptional cases (weird ones). Unless this is confirmed or corrected, let's not update the PR |
sergey-miryanov commented Sep 18, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
OK, maybe we can run refleak bots on this? Because |
kumaraditya303 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, this is correct, see the issue for my explanation for thread handle false leak.
1588413 into python:mainUh oh!
There was an error while loading. Please reload this page.
kumaraditya303 commented Oct 1, 2025
There seems to be a refleak in newly added ❯ ./python.exe -m test -R 3:3 test_capi.test_miscUsing random seed: 19231442390:00:00 load avg: 2.46 Run 1 test sequentially in a single process0:00:00 load avg: 2.46 [1/1] test_capi.test_miscbeginning 6 repetitions. Showing number of leaks (. for 0 or less, X for 10 or more)123:456XXX XXXtest_capi.test_misc leaked [20, 20, 20] references, sum=60test_capi.test_misc leaked [14, 14, 14] memory blocks, sum=420:00:54 load avg: 1.88 [1/1/1] test_capi.test_misc failed (reference leak) in 54.8 sec== Tests result: FAILURE ==1 test failed: test_capi.test_miscTotal duration: 54.8 secTotal tests: run=270 skipped=4Total test files: run=1/1 failed=1Result: FAILUREI missed it as it didn't show up until I merged main into this locally while working on #139473. |
ZeroIntensity commented Oct 1, 2025
There was an FD leak on that test before, but we fixed it. Is it possible that this PR just had an old version of Otherwise, that looks like an actual leak. We shouldn't just put it in a subprocess to hide it. |
kumaraditya303 commented Oct 1, 2025
Likely as I did run the tests myself before merging.
Yeah, will revert this for now and investigate, the subprocess change I was doing was orthogonal to this and wasn't meant to hide it. |
kumaraditya303 commented Oct 1, 2025
Created partial revert PR here #139474 |
vstinner commented Oct 1, 2025
Well, this leak confirms what I noticed when I created the issue:
|
kumaraditya303 commented Oct 1, 2025
Hmm, I still think that the underlying issue is different and is an artifact of how refleak checker works, for example the os.register_at_fork never works correctly with the checker. I will look more into this after the revert. |
Remove gc-support from some immutable types (see discussion in linked issue):
Also fixed: