Skip to content

fix(io): break PyArrowFileIO reference cycle so filesystems are freed by refcounting - #4070

Open
richacode007-byte wants to merge 2 commits into
apache:mainfrom
richacode007-byte:fix/4069-pyarrow-fileio-ref-cycle
Open

richacode007-byte wants to merge 2 commits into
apache:mainfrom
richacode007-byte:fix/4069-pyarrow-fileio-ref-cycle

Conversation

@richacode007-byte

Copy link
Copy Markdown

Closes #4069

Rationale for this change

PyArrowFileIO.__init__ caches filesystems with lru_cache(self._initialize_fs). The cache wraps a bound method, which holds a strong reference back to self. Since self also holds the cache, every PyArrowFileIO is 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 an lru_cache-wrapped closure that holds only a weakref to 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_scheme still behaves exactly as before: same signature, same caching, and cache_info() still works.
  • __getstate__ is unchanged. __setstate__ rebuilds the cache through the same factory, so unpickled instances don't have the cycle either.
  • Tests that patch PyArrowFileIO._initialize_fs on the class keep working, because the closure calls it through the instance.

Out of scope, possible follow-ups:

  • FsspecFileIO may have the same pattern.
  • SqlCatalog creates 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 with fs_by_scheme("file", None), drops the FileIO, and checks that a weakref to 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 a LocalFileSystem and caches it, and checks it's also freed by refcounting alone.

Both tests fail on main and pass with this change. The existing test_pyarrow_file_io_fs_by_scheme_cache test, which uses cache_info(), still passes. make test passes (4255 passed, 5 skipped), and make lint passes.

Are there any user-facing changes?

No API changes. The only difference users will see is that a PyArrowFileIO and its filesystems (with their connections) are released as soon as nothing references them, instead of waiting for the cycle collector.

@richacode007-byte

Copy link
Copy Markdown
Author

@ragnard @yiftizur @gardenia Pls review the code

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PyArrowFileIO is only freed by the cycle collector, so S3 connections pile up on Python 3.14

1 participant