fix(db): resolve resource leakage in database close()#6688
fix(db): resolve resource leakage in database close()#6688warku123 wants to merge 14 commits intotronprotocol:developfrom
close()#6688Conversation
Split the try-catch block to ensure parent resources are released even when writeOptions.close() throws exception. - Add proper cleanup in finally block to prevent cross-test contamination - Add Javadoc annotations for test class and methods - Fix checkstyle violations (import order and line length) Test: Add unit test to verify super.close() is always called
Split the single try-catch block in TronDatabase.close() into separate blocks for writeOptions and dbSource to ensure dbSource.closeDB() runs even when writeOptions.close() throws an exception. Rewrite tests to verify the exception scenario: mock writeOptions to throw and assert that dbSource.closeDB() is still called.
close()
| logger.warn("Failed to close dbSource in {}.", getName(), e); | ||
| } | ||
| logger.info("******** End to close {}. ********", getName()); | ||
| } |
There was a problem hiding this comment.
The fix for splitting the try-catch blocks is clean and directly addresses the root cause — really nice work isolating each resource cleanup independently!
One small thing: the "End to close" log moved from a finally block to a bare statement after both try-catch blocks. In the (admittedly rare) event that an unchecked Error (e.g. OutOfMemoryError) escapes from dbSource.closeDB(), this line won't execute — whereas the original finally guaranteed it would. Would it be worth wrapping both try-catch blocks in an outer try-finally?
try {
try { writeOptions.close(); } catch (Exception e) { ... }
try { dbSource.closeDB(); } catch (Exception e) { ... }
} finally {
logger.info("******** End to close {}. ********", getName());
}There was a problem hiding this comment.
Thanks for the suggestion! Good catch on the Error case. I've moved the log into a finally on the second try-catch block instead of adding an outer try-finally — same guarantee, slightly less nesting. Updated in bcb7c0b.
There was a problem hiding this comment.
Nice, putting the log in the second finally is a clean alternative — less nesting with the same guarantee. Looks good!
Move the "End to close" log back into a finally block so it is guaranteed to execute even if dbSource.closeDB() throws an Error.
| logger.debug("******** End to close {}. ********", getName()); | ||
| logger.warn("Failed to close writeOptions in {}.", getName(), e); | ||
| } | ||
| super.close(); |
There was a problem hiding this comment.
[[SHOULD]]CheckPointV2Store.close() previously logged at debug on purpose — SnapshotManager.createCheckpoint() opens/closes one on every checkpoint flush. Delegating to super.close() promotes those lines to info and will spam logs.
Suggest a
protected doClose()hook in the parent so the subclass can keep itsdebuglevel without duplicating logs:
// TronDatabase
@Override
public void close() {
logger.info("******** Begin to close {}. ********", getName());
doClose();
logger.info("******** End to close {}. ********", getName());
}
protected void doClose() {
try { writeOptions.close(); }
catch (Exception e) { logger.warn("Failed to close writeOptions in {}.", getName(), e); }
try { dbSource.closeDB(); }
catch (Exception e) { logger.warn("Failed to close dbSource in {}.", getName(), e); }
}// CheckPointV2Store
@Override
public void close() {
logger.debug("******** Begin to close {}. ********", getName());
try { writeOptions.close(); }
catch (Exception e) { logger.warn("Failed to close writeOptions in {}.", getName(), e); }
doClose();
logger.debug("******** End to close {}. ********", getName());
}There was a problem hiding this comment.
Good catch! Implemented the doClose() hook in 39cddb5 — CheckPointV2Store.close() now logs at debug level and calls doClose() directly, so high-frequency checkpoint flushes won't spam the info log.
…ug level TronDatabase.close() now delegates resource cleanup to a protected doClose() method, keeping info-level logging in the base class. CheckPointV2Store.close() overrides with debug-level logging and calls doClose() directly, avoiding log spam on high-frequency checkpoint flushes.
4a979db to
22dd4a1
Compare
What does this PR do?
Fix resource leakage in
CheckPointV2Store.close()andTronDatabase.close()by ensuringdbSource.closeDB()is always executed, even whenwriteOptions.close()throws an exception.Changes:
TronDatabase.close(): Split the single try-catch block into two independent try-catch blocks forwriteOptions.close()anddbSource.closeDB(), so that a failure in one does not prevent the other from executing.CheckPointV2Store.close(): Close the subclass-specificwriteOptionsfirst, then delegate tosuper.close()for parent resource cleanup, removing duplicated close logic and logging.CheckPointV2StoreTest: Add two test cases:testCloseReleasesAllResources— verifies thatdbSource.closeDB()is called during normal close.testCloseDbSourceWhenWriteOptionsThrows— mocks both child and parentwriteOptions.close()to throw exceptions, then verifiesdbSource.closeDB()is still called.Why are these changes required?
Previously,
writeOptions.close()anddbSource.closeDB()were in the same try block in bothTronDatabase.close()andCheckPointV2Store.close(). IfwriteOptions.close()threw an exception,dbSource.closeDB()would be skipped, causing the underlying database (LevelDB/RocksDB) to remain open and leak native resources. This is particularly problematic in environments where multiple database instances are created and destroyed.This PR has been tested by:
CheckPointV2StoreTest— both happy path and exception scenario)Follow up
N/A
Extra details
This is a revised version of #6621, addressing all review feedback:
TronDatabase.close()(parent class), not just the subclassdbSource.closeDB()is called, which is the core guarantee of this fix