fix: HoodieStorage resource leak in FileSystemBasedLockProvider.close() by mailtoboggavarapu-coder · Pull Request #18461 · apache/hudi

mailtoboggavarapu-coder · 2026-04-03T20:38:41Z

Describe the issue this Pull Request addresses

FileSystemBasedLockProvider creates a HoodieStorage instance in its constructor but never closes it in the close() method. Since HoodieStorage implements Closeable and holds underlying filesystem connections, this causes a resource leak every time the lock provider is closed.

Summary and Changelog

Fixed a resource leak in FileSystemBasedLockProvider.close() by ensuring HoodieStorage is always closed.

FileSystemBasedLockProvider.java: Added a finally block in close() to call storage.close() after the lock file deletion attempt. IOException from storage.close() is caught and logged as a warning to avoid masking the original exception.

Impact

No public API or user-facing change. Prevents filesystem connection accumulation in long-running jobs using the filesystem-based lock provider.

Risk Level

low — Only affects resource cleanup; no functional locking logic changed.

Documentation Update

none

Contributor's checklist

Read through contributor's guide
Enough context is provided in the sections above
Adequate tests were added if applicable

mailtoboggavarapu-coder

Pinging for an initial review. This PR fixes a genuine resource leak: FileSystemBasedLockProvider creates a HoodieStorage (which holds underlying filesystem connections and implements Closeable) in its constructor but never closes it in close(). The fix is a one-liner — calling storage.close() inside the existing close() method so the resource is released every time the lock provider is torn down.

Would appreciate a review from a committer with write access to apache/hudi (@vinothchandrasekar / @alexeykudinkin / @danny0405 / @nsivabalan).

danny0405 · 2026-04-15T02:37:26Z

+                try {
+                          storage.close();
+                                  } catch (IOException e) {
+                                            LOG.warn("Failed to close storage in FileSystemBasedLockProvider", e);


good catch, can you fix the indentation.

mailtoboggavarapu-coder · 2026-04-15T23:32:21Z

CI Build Failures — Master Branch Issue (not this PR)

The Java CI build failures on this PR are not caused by our code changes. Investigation shows this is a master branch build issue introduced by commit d3e0201 ("fix(common): FutureUtils:allOf should always throw root cause exception", merged 2026-04-15T22:30Z).

Evidence:

The master branch CI run for d3e0201 (run 24481746190) shows the same "Build Project" step failures in test-common-and-other-modules and test-spark-java-tests-part2 jobs
Our code changes touch completely unrelated files (DFSPropertiesConfiguration.java / HiveIncrementalPuller.java / FileSystemBasedLockProvider.java / SqlFileBasedSource.java) — none of which interact with FutureUtils
The last successful PR CI run before d3e0201 (nsivabalan's run at 21:53 UTC same day) passed all 53/53 jobs with identical matrix configuration
Build failures occur in ~83–111 seconds (a full Maven build normally takes 340s+), consistent with an early Maven resolution/plugin failure rather than a compilation error in our code

The CI situation is being tracked. Once the master build stabilises, a re-run of CI on this PR should pass.

cc @vinothchandrasekar @alexeykudinkin @danny0405 @nsivabalan — would appreciate a re-run once the master build issue is resolved.

danny0405 · 2026-04-16T01:27:40Z

There is compile error:

/home/runner/work/hudi/hudi/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/FileSystemBasedLockProvider.java:[118,10] class, interface, enum, or record expected

mailtoboggavarapu-coder · 2026-04-16T13:19:14Z

@danny0405 Thank you for merging #18457 and #18467!

This PR (#18461) fixes a HoodieStorage resource leak in FileSystemBasedLockProvider.close() — the storage object was never closed. Branch synced with latest master, fresh CI triggered (run 24512491429). Would appreciate a review!

mailtoboggavarapu-coder · 2026-04-16T13:22:13Z

CI Re-triggered After Master Fix

Master has advanced past the broken d3e020132a commit (the FutureUtils:allOf change that broke Spark CI). Additionally, PRs #18457 and #18467 from this series have already been merged into master.

This branch has been synced with the updated master and an empty commit was pushed to re-trigger CI. CI results on this run should be clean.

Adds a finally block to properly close the HoodieStorage instance after the lock file is deleted, preventing resource leaks. Fixes apache#14922

codecov-commenter · 2026-04-16T16:59:51Z

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.86%. Comparing base (5b68607) to head (a88509f).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
.../transaction/lock/FileSystemBasedLockProvider.java	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #18461   +/-   ##
=========================================
  Coverage     68.85%   68.86%           
- Complexity    28241    28256   +15     
=========================================
  Files          2460     2460           
  Lines        135348   135351    +3     
  Branches      16410    16407    -3     
=========================================
+ Hits          93200    93206    +6     
- Misses        34770    34771    +1     
+ Partials       7378     7374    -4

Flag	Coverage Δ
common-and-other-modules	`44.58% <50.00%> (+<0.01%)`	⬆️
hadoop-mr-java-client	`44.85% <50.00%> (+<0.01%)`	⬆️
spark-client-hadoop-common	`48.51% <0.00%> (-0.01%)`	⬇️
spark-java-tests	`48.96% <50.00%> (-0.01%)`	⬇️
spark-scala-tests	`45.49% <50.00%> (+<0.01%)`	⬆️
utilities	`38.21% <50.00%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
.../transaction/lock/FileSystemBasedLockProvider.java	`60.86% <50.00%> (-0.93%)`	⬇️

... and 8 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mailtoboggavarapu-coder · 2026-04-16T17:21:04Z

Hi @danny0405, all 55 CI checks are now passing ✅ for this PR.

This fix ensures HoodieStorage in FileSystemBasedLockProvider is properly closed in a finally block to prevent resource leaks even when exceptions occur.

Would you be able to review and merge when you get a chance? Thanks!

hudi-bot · 2026-04-16T17:52:27Z

CI report:

66523bc UNKNOWN
fcd32b4 UNKNOWN
f9c08ff UNKNOWN
a88509f Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run azure re-run the last Azure build

yihua

🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.

Thanks for contributing! The change is safe and follows good resource management practice. One thing worth noting: HoodieHadoopStorage.close() is intentionally a no-op to avoid closing the JVM-cached Hadoop FileSystem, so the actual resource leak fix may not have impact with the default storage backend today — see inline comment for details.

yihua · 2026-04-16T17:56:54Z

        throw new HoodieLockException(generateLogStatement(LockState.FAILED_TO_RELEASE), e);
+      } finally {
+        try {
+          storage.close();


🤖 Worth noting that HoodieHadoopStorage.close() is intentionally a no-op (it avoids closing the cached Hadoop FileSystem). So this won't actually release resources with the default storage implementation today. Still reasonable to add for correctness with potential future storage backends, but wanted to flag this so the impact is clear.

_{- Generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.}

yihua

🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.

Style & Readability Review — one minor suggestion to simplify the log message.

yihua · 2026-04-16T18:03:37Z

+        try {
+          storage.close();
+        } catch (IOException closeEx) {
+          log.warn("Failed to close HoodieStorage in FileSystemBasedLockProvider", closeEx);


🤖 nit: could you simplify the log message to just 'Failed to close HoodieStorage'? The logger already provides the class context automatically.

_{- Generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.}

yihua

🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.

LGTM — clean fix for a genuine resource leak. The finally block correctly ensures storage.close() runs regardless of the deleteFile outcome, and the caught IOException avoids masking the original exception.

github-actions bot added the size:XS PR with lines of changes in <= 10 label Apr 3, 2026

mailtoboggavarapu-coder commented Apr 14, 2026

View reviewed changes

danny0405 reviewed Apr 15, 2026

View reviewed changes

mailtoboggavarapu-coder changed the title ~~Fix HoodieStorage resource leak in FileSystemBasedLockProvider.close()~~ fix: HoodieStorage resource leak in FileSystemBasedLockProvider.close() Apr 15, 2026

mailtoboggavarapu-coder mentioned this pull request Apr 15, 2026

fix(common): FutureUtils:allOf should always throw root cause exception #18456

Merged

3 tasks

mailtoboggavarapu-coder force-pushed the fix/filesystem-lock-provider-storage-leak branch 3 times, most recently from f9c08ff to cb096a1 Compare April 16, 2026 14:47

fix: close HoodieStorage resource in FileSystemBasedLockProvider.close()

a88509f

Adds a finally block to properly close the HoodieStorage instance after the lock file is deleted, preventing resource leaks. Fixes apache#14922

mailtoboggavarapu-coder force-pushed the fix/filesystem-lock-provider-storage-leak branch from cb096a1 to a88509f Compare April 16, 2026 15:35

yihua reviewed Apr 16, 2026

View reviewed changes

Conversation

mailtoboggavarapu-coder commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe the issue this Pull Request addresses

Summary and Changelog

Impact

Risk Level

Documentation Update

Contributor's checklist

Uh oh!

mailtoboggavarapu-coder left a comment

Choose a reason for hiding this comment

Uh oh!

danny0405 Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

mailtoboggavarapu-coder commented Apr 15, 2026

CI Build Failures — Master Branch Issue (not this PR)

Uh oh!

danny0405 commented Apr 16, 2026

Uh oh!

mailtoboggavarapu-coder commented Apr 16, 2026

Uh oh!

mailtoboggavarapu-coder commented Apr 16, 2026

CI Re-triggered After Master Fix

Uh oh!

codecov-commenter commented Apr 16, 2026

Codecov Report

Uh oh!

mailtoboggavarapu-coder commented Apr 16, 2026

Uh oh!

hudi-bot commented Apr 16, 2026

CI report:

Uh oh!

yihua left a comment

Choose a reason for hiding this comment

Uh oh!

yihua Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

yihua left a comment

Choose a reason for hiding this comment

Uh oh!

yihua Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

yihua left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mailtoboggavarapu-coder commented Apr 3, 2026 •

edited

Loading