| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
As @picnixz suggested this should skip news. |
Sorry, something went wrong.
|
This ready to review, please take a look. |
Sorry, something went wrong.
|
I'll have a look tomorrow or later tonight |
Sorry, something went wrong.
|
Be careful not to create ref cycles. At least, PyType_FromModuleAndSpec() is supposed to have a NULL module argument without GC, in which case it's impossible to get the module state from the heaptype. |
Sorry, something went wrong.
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) |
Sorry, something went wrong.
|
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: _bz2.BZ2Compressor GC required by clinic_state()? _bz2.BZ2Decompressor GC required by clinic_state()? _hashlib.HASH lgtm _hashlib.HASHXOF lgtm _hashlib.HMAC lgtm _interpchannels.ChannelID PyType_FromSpec() instead? _lzma.LZMACompressor GC required? _lzma.LZMADecompressor GC required? _ssl.Certificate GC required? _thread._localdummy lgtm _tokenize.TokenizerIter PyType_FromSpec() instead? |
Sorry, something went wrong.
|
I'm not sure I get it. IIUC, the BZ2Compressor didn't support GC from the beginning. tp_traverse was added in #20960 as suggested by @vstinner. However, IIUC, tp_traverse is not called outside of the gc module. And BZ2Compressor will never be a part of any GC list, so tp_traverse will never be called for it. Even if BZ2Compressor holds a strong ref to module (via ht_module), it is never handled by the gc cycle-machinery. Maybe I'm missing something? |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
tp_traverse is reachable if BZ2Compressor have Py_TPFLAGS_HAVE_GC: import gc, bz2
_ = bz2.BZ2Compressor()
gc.collect()It seems like #20960 just forgot to add the GC flag when the isolation howto is not yet established. |
Sorry, something went wrong.
|
Then we should restore GC-support for these types. @picnixz OK? |
Sorry, something went wrong.
|
And maybe check for other too. |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
OK, maybe we can run refleak bots on this? Because test_capi doesn't leak for me: [2025-09-18 21:54:48] [D:\Sources\_pythonish\cpython\main] [116946-remove-gc-from-immutable-types +73 ~0 -0 !] [ 10.2] ➜ .\python.bat -m test test_capi -R : Running Debug|x64 interpreter... Using random seed: 2150303110 0:00:00 Run 1 test sequentially in a single process 0:00:00 [1/1] test_capi beginning 9 repetitions. Showing number of leaks (. for 0 or less, X for 10 or more) 12345:6789 XX... .... 0:03:15 [1/1] test_capi passed in 3 min 15 sec == Tests result: SUCCESS == 1 test OK. Total duration: 3 min 15 sec Total tests: run=1,167 skipped=132 Total test files: run=1/1 Result: SUCCESS |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM, this is correct, see the issue for my explanation for thread handle false leak.
Sorry, something went wrong.
|
There seems to be a refleak in newly added test_pending_call_creates_thread_subinterpreter with this change: ❯ ./python.exe -m test -R 3:3 test_capi.test_misc
Using random seed: 1923144239
0:00:00 load avg: 2.46 Run 1 test sequentially in a single process
0:00:00 load avg: 2.46 [1/1] test_capi.test_misc
beginning 6 repetitions. Showing number of leaks (. for 0 or less, X for 10 or more)
123:456
XXX XXX
test_capi.test_misc leaked [20, 20, 20] references, sum=60
test_capi.test_misc leaked [14, 14, 14] memory blocks, sum=42
0: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_misc
Total duration: 54.8 sec
Total tests: run=270 skipped=4
Total test files: run=1/1 failed=1
Result: FAILURE
I missed it as it didn't show up until I merged main into this locally while working on #139473. |
Sorry, something went wrong.
|
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 main? Otherwise, that looks like an actual leak. We shouldn't just put it in a subprocess to hide it. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
…ython#139073)" This reverts commit 1588413.
|
Created partial revert PR here #139474 |
Sorry, something went wrong.
|
Well, this leak confirms what I noticed when I created the issue:
|
Sorry, something went wrong.
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. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Remove gc-support from some immutable types (see discussion in linked issue):
Also fixed: