richacode007-byte opened a new pull request, #4070:
URL: https://github.com/apache/iceberg-python/pull/4070
<!--
Thanks for opening a pull request!
-->
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.
<!-- In the case of user-facing changes, please add the changelog label. -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
