[common] Fix resource leak and silenced exceptions in FileLock#3368
Open
XuQianJin-Stars wants to merge 1 commit into
Open
[common] Fix resource leak and silenced exceptions in FileLock#3368XuQianJin-Stars wants to merge 1 commit into
XuQianJin-Stars wants to merge 1 commit into
Conversation
FileLock#tryLock() previously caught every Exception and returned false, which had two issues: (1) genuine I/O failures were indistinguishable from inter-process lock contention, hiding real problems from callers, and (2) when the lock acquisition failed after init() had opened the FileOutputStream, the stream was never closed, leaking the underlying file descriptor for the lifetime of the JVM. FileLock#unlockAndDestroy() also chained the channel close and stream close in a single try block; if closing the channel threw, the FileOutputStream was never closed. This change: * Catches OverlappingFileLockException explicitly to keep the documented "return false on contention" semantics, but propagates IOException / RuntimeException so callers can react to real failures. * Closes the FileOutputStream when tryLock() fails after init(), preventing the descriptor leak. * Reworks unlockAndDestroy() with nested try-finally blocks so the output stream is always closed even when releasing or closing the channel throws. * Adds FileLockTest covering the happy path, in-JVM contention, reuse after unlockAndDestroy(), destroy without acquisition, isValid() before tryLock(), and the constructor's rejection of file names with no legal characters.
6a62af0 to
3147b48
Compare
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
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.
Purpose
FileLock#tryLock()previously caught everyExceptionand returnedfalse, which had two issues: (1) genuine I/O failures were indistinguishable from inter-process lock contention, hiding real problems from callers; and (2) when lock acquisition failed afterinit()had opened theFileOutputStream, that stream was never closed, leaking the underlying file descriptor for the lifetime of the JVM.FileLock#unlockAndDestroy()also chainedunlock(), channel close and stream close inside a singletryblock; if releasing the lock or closing the channel threw, theFileOutputStreamwas never closed and the lock file might not be deleted.This PR fixes both bugs and adds a dedicated test class for
FileLock.Linked issue: close #xxx
Brief change log
FileLock#tryLock()OverlappingFileLockExceptionexplicitly to preserve the documented "returnfalseon contention" semantics.IOException/RuntimeExceptionso callers can react to real failures instead of silently treating them as contention.FileOutputStreamopened byinit()whenever lock acquisition fails, preventing the file-descriptor leak.FileLock#unlockAndDestroy()try/finallyblocks so the channel and the output stream are each closed independently, and the lock file is always deleted at the end — even if releasing the lock or closing the channel throws.NetUtilsTest(the only production caller) still passes unchanged.Tests
Added
FileLockTestinfluss-common, covering:tryLock()→unlock()→unlockAndDestroy()and lock file is removed.tryLock()on the same path returnsfalseand does not throw or leak the stream.FileLockinstance afterunlockAndDestroy()(re-acquire works).unlockAndDestroy()is safe to call whentryLock()was never invoked.isValid()returnsfalsebefore anytryLock().Existing tests:
NetUtilsTest(the only production user ofFileLock) continues to pass.API and Format
FileLockare preserved; only internal exception handling and resource cleanup are corrected.Documentation
tryLock()already states thatfalsemeans the lock is held elsewhere; the new behavior aligns the implementation with that contract (real I/O errors are no longer hidden behindfalse).