Skip to content

[common] Fix file system leaks when the Hadoop file system cache is disabled - #8962

Open
wombatu-kun wants to merge 2 commits into
apache:masterfrom
wombatu-kun:issue/8548-hadoop-fileio-close-owned-filesystems
Open

[common] Fix file system leaks when the Hadoop file system cache is disabled#8962
wombatu-kun wants to merge 2 commits into
apache:masterfrom
wombatu-kun:issue/8548-hadoop-fileio-close-owned-filesystems

Conversation

@wombatu-kun

@wombatu-kun wombatu-kun commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Purpose

Closes #8548. With fs.<scheme>.impl.disable.cache=true every HadoopFileIO creates its own FileSystem, and nothing ever released them, so connector threads accumulate (reported with AliyunOSSFileSystem).

HadoopFileIO.close() now releases the file systems it owns, mirroring Hadoop's own branch in FileSystem.get(URI, Configuration): ours only when fs.<scheme>.impl.disable.cache is set for the path's scheme, matched as written. Otherwise the instance lives in Hadoop's global cache, is shared JVM wide, and must not be closed here. That decision is recorded when the file system is created and stored next to it, since the Configuration it comes from stays mutable.

Supporting fixes, without which that close is unreachable:

  • ResolvingFileIO and PluginFileIO did not forward close() to their delegates, unlike CachingFileIO. As a result OSSFileIO.close() had never run in production.
  • HadoopSecuredFileSystem did not override close().
  • FileIO.checkAccess discarded the FileIO it loads to test access, leaking one file system per FileIO.get(). It now hands that instance back and get returns it, which also stops the file system being built twice. Its return type changes from FileIOLoader to FileIO, a @Public signature change with no other callers in the repo.

close() is terminal, otherwise the wrappers silently re-created their delegates and the leak returned.

Making close() real also activates RESTTokenFileIO's eviction listener, and that JVM wide cache hands out raw FileIOs and streams. Its values are reference counted now: a lease is held for each operation and for the lifetime of every returned stream, and the delegate is closed only when the last one goes. BaseMultiPartUploadCommitter, LanceUtils and VortexUtils work inside a lease rather than unwrapping the cached instance, which is why those modules appear in the diff.

Out of scope: the per-module HadoopCompliantFileIO copies take file systems from static caches and need a separate design. Three pre-existing stream leaks found while reviewing this are filed as #9005, #9006 and #9007: they stop the fix reaching those cache entries, but are not regressions.

Tests

HadoopFileIOTest and RESTTokenFileIOTest (new) plus cases in FileIOTest, ResolvingFileIOTest, PluginFileIOTest and HadoopSecuredFileSystemTest: owned closed, cached untouched and still shared, ownership per scheme, ownership recorded at creation and honoured after the flag flips both ways, failing close does not skip the rest, idempotency, creation race, use after close, access probe reused rather than leaked and released when its loader is rejected or selection throws, eviction while leased, open streams keeping their delegate alive, the lease released once through either two phase ending.

@wombatu-kun
wombatu-kun force-pushed the issue/8548-hadoop-fileio-close-owned-filesystems branch from 62d8e28 to 28f7bf0 Compare July 31, 2026 12:07

@JingsongLi JingsongLi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found three lifecycle and ownership issues that should be addressed before merge.

// the delegate lives in the plugin classloader, so close it under that classloader too
wrap(
() -> {
fileIO.close();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Keep REST-token cache entries alive while they are still borrowed

This forwarding close makes RESTTokenFileIO's existing removal listener actually close the plugin delegate. That cache returns raw FileIO values and streams without a lease or reference count, so size eviction after more than 1,000 token entries (or expiry) can close an uncached OSS filesystem while another table is still reading or writing through it. AliyunOSSFileSystem.close() shuts down the OSS client and executor pools, so the active operation can fail mid-flight. Please defer the physical close until active operations and returned streams release their leases, or otherwise add equivalent lifetime tracking around cached values.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done bd9a8d9. Cache values are reference counted now: the removal listener hands back only the cache's own reference, and the delegate is closed when the last lease goes, with leases held for the duration of each operation and for the lifetime of every returned stream.

The window is wider than the entry count suggests - the admission policy can evict a just-inserted entry, so the caller's lease is taken before the put. fileIO() is deprecated because a raw reference carries no lifetime to track; BaseMultiPartUploadCommitter, LanceUtils and VortexUtils now work inside a lease instead.

return;
}
for (Map.Entry<Pair<String, String>, FileSystem> entry : map.entrySet()) {
if (isOwnedScheme(entry.getKey().getLeft())) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Preserve the ownership decision made when the filesystem was created

FileSystem.get decides cached-vs-owned using fs.<scheme>.impl.disable.cache at creation time, but this code recomputes ownership from the current mutable Configuration. SerializableConfiguration retains the caller's configuration by reference, hadoopConf() exposes it, and configure() can replace it. If a shared cached filesystem is created with the flag false and the flag later becomes true, close() misclassifies and closes a JVM-global filesystem still used by other readers; the reverse transition leaks an owned filesystem. Please store {fileSystem, ownedAtCreation} atomically in the map and use that recorded bit for loser cleanup and final close.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done bd9a8d9. The map holds {fileSystem, ownedAtCreation} and both the loser cleanup and close() read that bit. The same pass stops recording an externally injected file system as owned, releases the raw instance when the Kerberos wrapper fails, and moves the loser's close outside the monitor so a slow object store teardown cannot stall the other callers.

io.configure(config);
io.exists(path);
} finally {
IOUtils.closeQuietly(io);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Do not assume a public FileIOLoader returns a fresh instance on every call

checkAccess now terminally closes the probed instance, but after this method returns FileIO.get calls loader.load(path) again. The @Public FileIOLoader.load(Path) contract does not require a fresh instance, so a valid prefer/fallback loader that caches or returns a singleton will return the same now-closed PluginFileIO, HadoopFileIO, or ResolvingFileIO; every subsequent operation then fails with This FileIO is closed. Please carry the successfully probed instance through selection and return it, closing it only if that candidate is rejected.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done bd9a8d9. checkAccess returns the instance it checked and FileIO.get hands that one back, which also stops the file system being built twice. That changed its return type from FileIOLoader to FileIO, a signature change on a @Public interface with no other callers in the repo. The selection is also wrapped now, because an unchecked failure from a loader's requiredOptions() or getScheme() would otherwise strand the checked instance.

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.

[Bug] HadoopFileIO does not close per-instance FileSystems, causing executor thread growth with disabled Hadoop FS cache

2 participants