fix(db): resolve resource leakage in database close()#6688
fix(db): resolve resource leakage in database close()#6688warku123 wants to merge 15 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
| @Before | ||
| public void before() throws IOException { | ||
| EventPluginLoader.getInstance().setFilterQuery(null); | ||
| Args.getInstance().setNodeListenPort(10000 + port.incrementAndGet()); |
There was a problem hiding this comment.
Defensive reset unrelated to this PR's resource-leakage fix, but needed to stabilize the JaCoCo overall-delta gate.
Root cause: FilterQueryTest (in org.tron.common) sets EventPluginLoader.filterQuery = {fromBlock=100, toBlock=190} at the end of testMatchFilter() with no cleanup. With forkEvery=100, this PR's new test methods shifted the JVM batch boundary, causing FilterQueryTest and BlockEventGetTest to land in the same JVM. Since start(config) does not reset filterQuery, the stale range filter caused matchFilter() to return false for block 1, skipping processTrigger entirely — 0/293 instructions covered in the PR run vs 277/293 on base.
Evidence:
- ❌ Before fix — Coverage Gate failed: https://github.com/tronprotocol/java-tron/actions/runs/24652783259/job/72081002383
- ✅ After fix — Coverage Gate passed: https://github.com/tronprotocol/java-tron/actions/runs/24767648239/job/72467952762
Fix: FilterQueryTest now resets filterQuery to null in @After, eliminating the pollution at its source. The @Before reset here is an additional guard against any other test leaving dirty singleton state.
Note: This singleton pollution pattern may exist elsewhere in the codebase — any test that writes to a static singleton (e.g. Args, EventPluginLoader, ConfigLoader) without resetting it in @After is a potential source of similar unexpected JaCoCo coverage drops when batch boundaries shift.
FilterQueryTest.testMatchFilter() sets filterQuery={fromBlock=100,
toBlock=190} on the EventPluginLoader singleton with no cleanup.
With forkEvery=100, tests sharing the same JVM inherit this stale
state, causing matchFilter() to return false for block 1 and
skipping processTrigger() entirely (0/293 instructions covered).
Add @after teardown in FilterQueryTest to reset filterQuery to null,
and add a defensive reset in BlockEventGetTest.@before as a guard
against any other preceding test leaving dirty singleton state.
6b37267 to
ab0db5f
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