-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
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
Run GC in file leak check #48294
Run GC in file leak check #48294
Conversation
does this merit a test? |
Happy to add one, but I think it is difficult to come up with one that is both not too contrived and guaranteed to work (the one above scores good on the former but bad on the latter) |
I'm not sure I conceptually agree with explicitly running the garbage collector. Is this not something we should just handle via weak references? |
That would require all references that might indirectly reference any open files to be weak references. Actually I think it’s likely not even the standard library guarantees this. The standard idiom is to use |
Ah sorry didn't notice the full context to this. Your explanation makes sense, though this seems symptomatic of other problems in the code base that ideally should be addressed |
Thanks @jonashaag |
This fixes a problem with
file_leak_context
(xref #30096 (comment)).Demo:
Here we have a reference cycle
reader -> .cycle -> reader
that is only cleared when GC is run. On my machine, without thegc.collect()
statement,file_leak_context
detects a file leak where there is none.The PR fixes this by explicitly calling
gc.collect()
before collecting the leaked files. I also changed an equality to a subset check to make the code robust against additional closed files (due to thegc.collect()
call, or other reasons that I don't have any examples for).cc @jbrockmendel
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.