Uh oh!
There was an error while loading. Please reload this page.
gh-116946: Remove gc-support from some immutable types - #139073
Conversation
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 |
OK, maybe we can run refleak bots on this? Because |
kumaraditya303
left a comment
There was a problem hiding this comment.
LGTM, this is correct, see the issue for my explanation for thread handle false leak.
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: