wombatu-kun opened a new pull request, #8962:
URL: https://github.com/apache/paimon/pull/8962
### Purpose
Closes #8548. With the Hadoop FileSystem cache disabled
(`fs.<scheme>.impl.disable.cache=true`, commonly used to isolate per-tenant
credentials), every `HadoopFileIO` creates its own `FileSystem`, and nothing
ever released them. The reporter sees unbounded JVM thread growth because each
unclosed `AliyunOSSFileSystem` keeps its transfer executor threads alive.
**The ownership rule.** `HadoopFileIO.createFileSystem` calls
`path.getFileSystem(conf)`, which is Hadoop's `FileSystem.get(URI,
Configuration)`:
```java
String disableCacheName = String.format("fs.%s.impl.disable.cache", scheme);
if (conf.getBoolean(disableCacheName, false)) {
return createFileSystem(uri, conf); // a fresh instance nobody else
can reach
}
return CACHE.get(uri, conf); // shared, owned by Hadoop's
global cache
```
So a `FileIO` owns a `FileSystem` if and only if that property is set for
the scheme, and `close()` releases exactly those. When the global cache is
enabled the instance is shared with every other user in the JVM, Hadoop
releases it through `FileSystem.closeAll()` on its shutdown hook, and closing
it here would surface as `IOException: Filesystem closed` in unrelated readers.
This answers the two design questions the reporter asked on the issue.
Two details are load bearing, both checked against the Hadoop bytecode
(2.6.2 and 3.1.0) rather than assumed. The scheme comes from the **path**, not
from `FileSystem#getUri()`, because that is what Hadoop consulted when it
decided whether to cache. And it is matched **as written**, with no lower
casing, because Hadoop does not lower case it either: a path spelled
`OSS://bucket` is served from the shared cache, so a case-insensitive check
would find `fs.oss.impl.disable.cache=true` and close an instance it does not
own.
**The close never reached `HadoopFileIO`.** Real deployments hold a
`ResolvingFileIO` or a `PluginFileIO`, and neither forwarded `close()` to its
delegates, so a `HadoopFileIO.close()` alone would have been dead code.
`CachingFileIO.close()` already forwards, which is the pattern the other two
now follow. Notably this also means `OSSFileIO.close()`, which the project
already wrote to release uncached file systems, has never actually run in
production, and it targets exactly the reporter's symptom.
`HadoopSecuredFileSystem` likewise extended `FileSystem` without overriding
`close()`, so on a Kerberised deployment the wrapped instance was never closed.
**A second, larger leak on the creation path.** `FileIO.checkAccess` probes
a loader by loading a throwaway `FileIO` and calling `exists()` on it, then
discards it. With the cache disabled that probe materialises a fully
initialised `FileSystem`, connector threads included, on **every**
`FileIO.get()` call for any scheme served by the universal `HadoopFileIOLoader`
fallback. This one leaks without anyone calling `close()` at all, and a
measurement of the reporter's configuration shows two file systems created and
one closed per `FileIO.get()`.
**Close is terminal.** Without a closed flag each wrapper silently
re-created its delegate after `close()`, restoring the very leak being fixed,
and a `FileSystem` published concurrently with a `close()` could land in a
drained map where nothing would ever release it. `HadoopFileIO` now publishes
under the same monitor `close()` takes, and use after close fails with a clear
`IOException` instead of leaking quietly.
**Out of scope, stated explicitly.** The seven per-module
`HadoopCompliantFileIO` copies under `paimon-filesystems` obtain their file
systems from module-level static caches (for example `S3FileIO.CACHE`), which
are process wide by design, so releasing those per instance needs a separate
refcounting design.
**One behaviour change reviewers should weigh.** `RESTTokenFileIO` keeps a
static cache of `FileIO`s whose removal listener already calls
`IOUtils.closeQuietly`. That call has been landing on the empty default
`close()`; now that `close()` does real work, a size based eviction can in
principle release a file system another table is still reading through. Two
things keep it narrow: the cache evicts on ten hour idle access or above 1000
entries, and the ownership gate means only instances created under
`disable.cache=true` are ever closed, which is the configuration where the
operator already opted into one file system per `FileIO`. `OSSFileIO` carries
the same exposure today, so this is new reach rather than a new class of
hazard. The `FileIO.close()` javadoc, which previously told implementers to
override the method to empty, is updated to match.
### Tests
New `HadoopFileIOTest` (16 tests) plus additions to `ResolvingFileIOTest`,
`PluginFileIOTest` and `HadoopSecuredFileSystemTest`, built on a recording
`FileSystem` registered under synthetic schemes. Coverage: an owned file system
is closed and a Hadoop cached one is not (proved by pulling the same instance
back through a second `FileIO`, not just by a counter); ownership is decided
per scheme and follows the path rather than `getUri()`; the scheme is matched
as written; a failing `close()` does not stop the others and still propagates;
close is idempotent and safe before any use, on a deserialized instance, and on
an externally injected file system the caller still owns; the creation race
releases the losing instance while never closing a shared one; a file system
created while closing is released; a malformed `fs.defaultFS` cannot throw out
of `close()`; use after close is rejected rather than leaking again; and
`FileIO.get` leaves no unclosed access probe.
Verified by reverting the implementation while keeping the API: the tests
asserting new behaviour fail, while those guarding the dangerous half (a shared
file system must stay open) keep passing.
Regression runs on JDK 8: full `paimon-common` suite 12198 tests green,
`paimon-core` rest, catalog and fs packages 342 tests green, checkstyle and
spotless clean.
Audited before submitting, with independent reviewers on ownership
semantics, concurrency and test adequacy, and every finding verified against
the code. That pass found and fixed a defect in my own first draft: making
`close()` null the `PluginFileIO` delegate turned `fileIO()` into a method that
could return null to a caller about to dereference it. It also found the
`checkAccess` probe leak described above.
--
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]
