Close Files.walk stream in LocalLogFileServer to prevent file descriptor leak - #19242
Open
dkranchii wants to merge 1 commit into
Open
Close Files.walk stream in LocalLogFileServer to prevent file descriptor leak#19242dkranchii wants to merge 1 commit into
dkranchii wants to merge 1 commit into
Conversation
…tor leak LocalLogFileServer.getAllLogFilePaths() enumerated log files via Files.walk(_logRootDirPath).filter(...).forEach(...) with no try-with-resources block. Per the JDK Javadoc, the Stream returned by Files.walk encapsulates one or more DirectoryStreams, and the caller is responsible for closing it to release native file-descriptor resources. Because downloadLogFile(String) calls getAllLogFilePaths() on every download request, this leak amplified on hot paths: each call could retain a DirectoryStream beyond GC's discretion, gradually approaching the process ulimit -n on long-lived server and controller instances. Wrap the Files.walk stream in try-with-resources so the underlying DirectoryStream is released as soon as enumeration completes. Tests: - New testGetAllLogFilePathsEnumeratesNestedDirectories verifies that recursion into subdirectories still returns paths relative to the log root and that those paths remain downloadable through downloadLogFile, guarding the refactor against regressions. - Existing testLoggerFileServer continues to cover the flat-directory happy path and the FORBIDDEN response for unknown paths.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19242 +/- ##
============================================
+ Coverage 66.94% 66.95% +0.01%
Complexity 1423 1423
============================================
Files 3452 3452
Lines 218564 218620 +56
Branches 34731 34742 +11
============================================
+ Hits 146320 146386 +66
+ Misses 60549 60543 -6
+ Partials 11695 11691 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
@Jackie-Jiang can you review this pr. thanks |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
LocalLogFileServer.getAllLogFilePaths()enumerated log files viaFiles.walk(_logRootDirPath).filter(...).forEach(...)without atry-with-resourcesblock. Per the JDKFiles.walkjavadoc:Because
downloadLogFile(String)callsgetAllLogFilePaths()on every download request (to authorize the requested path), the leak amplifies on hot paths — long-lived server and controller instances gradually accumulateDirectoryStreamfile descriptors until they approach the processulimit -n.This PR wraps the
Files.walkstream in atry-with-resourcesblock so the underlyingDirectoryStream(s) are released as soon as enumeration completes. Sibling code in Pinot already uses the same pattern (e.g.LocalPinotFS.listFiles).Change
pinot-common/src/main/java/org/apache/pinot/common/utils/log/LocalLogFileServer.java— wrapFiles.walk(_logRootDirPath)intry (Stream<Path> paths = Files.walk(...)); enumeration logic is unchanged.Backwards compatibility
None affected. Public API, return values, and enumeration behavior are unchanged.
Tests
LocalLogFileServerTest#testGetAllLogFilePathsEnumeratesNestedDirectories— creates a nestedsub/dir/nested.log, asserts both files are enumerated with paths relative to the log root, and asserts both are downloadable viadownloadLogFile(...). This guards the refactor against a regression that would break recursion into subdirectories.testLoggerFileServercontinues to exercise the flat-directory happy path and theFORBIDDENresponse for unknown paths.Run locally:
./mvnw -pl pinot-common -am -Dtest=LocalLogFileServerTest testRisk
Very low. The change is a mechanical
try-with-resourceswrap around an existingFiles.walkinvocation; the traversal semantics are unchanged. The stream is fully consumed inside the block, so no lazy operations escape.