fix(io): break PyArrowFileIO reference cycle so filesystems are freed by refcounting - #4070
Open
richacode007-byte wants to merge 2 commits into
Open
richacode007-byte wants to merge 2 commits into
richacode007-byte wants to merge 2 commits into
Conversation
Author
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4069
Rationale for this change
PyArrowFileIO.__init__caches filesystems withlru_cache(self._initialize_fs). The cache wraps a bound method, which holds a strong reference back toself. Sinceselfalso holds the cache, everyPyArrowFileIOis part of a reference cycle.So when the last user reference to a FileIO goes away, refcounting can't free it. The FileIO and every filesystem it cached stay alive until Python's cycle collector happens to run, along with their open connections and connection pools (S3, GCS, ADLS, ...). In long-running processes that create many catalogs or tables, this shows up as connections and memory that pile up for no visible reason.
This PR replaces the bound-method cache with a small module-level factory,
_fs_by_scheme_cache(file_io). It returns anlru_cache-wrapped closure that holds only aweakrefto the FileIO. That removes the cycle, so the FileIO and its cached filesystems are freed by plain refcounting as soon as they're unused.fs_by_schemestill behaves exactly as before: same signature, same caching, andcache_info()still works.__getstate__is unchanged.__setstate__rebuilds the cache through the same factory, so unpickled instances don't have the cycle either.PyArrowFileIO._initialize_fson the class keep working, because the closure calls it through the instance.Out of scope, possible follow-ups:
FsspecFileIOmay have the same pattern.SqlCatalogcreates a second FileIO, which is worth checking separately.Are these changes tested?
Yes. Two new tests in
tests/io/test_pyarrow.py:test_pyarrow_file_io_freed_by_refcounting: turns off the cycle collector (gc.disable(), restored afterwards), creates a FileIO, fills the cache withfs_by_scheme("file", None), drops the FileIO, and checks that aweakrefto it is now dead.test_pyarrow_file_io_pickle_round_trip_keeps_cache_and_refcounting: pickles and unpickles a FileIO, checks the restored copy still resolves aLocalFileSystemand caches it, and checks it's also freed by refcounting alone.Both tests fail on
mainand pass with this change. The existingtest_pyarrow_file_io_fs_by_scheme_cachetest, which usescache_info(), still passes.make testpasses (4255 passed, 5 skipped), andmake lintpasses.Are there any user-facing changes?
No API changes. The only difference users will see is that a
PyArrowFileIOand its filesystems (with their connections) are released as soon as nothing references them, instead of waiting for the cycle collector.