fix(db): resolve resource leakage in database close()#21
Open
fix(db): resolve resource leakage in database close()#21
Conversation
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.
Move the "End to close" log back into a finally block so it is guaranteed to execute even if dbSource.closeDB() throws an Error.
…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.
📝 WalkthroughWalkthroughThe pull request refactors database close mechanisms in Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bladehan1
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
framework/src/test/java/org/tron/core/db/CheckPointV2StoreTest.java (2)
71-87:testCloseWhenDbSourceThrowshas no explicit assertion.The test verifies behavior only implicitly — it passes as long as
store.close()does not propagate the exception. Addingverify(mockDbSource).closeDB();would make the contract explicit (ensures the throwing path was actually exercised, not silently skipped due to a future refactor), and anAssert.fail-style guard could catch accidental propagation:🧪 Proposed assertion
try { store.close(); + verify(mockDbSource).closeDB(); } finally { originalDbSource.closeDB(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/core/db/CheckPointV2StoreTest.java` around lines 71 - 87, The test testCloseWhenDbSourceThrows currently lacks explicit assertions—update it to verify that the mocked DbSourceInter's closeDB() was invoked and to explicitly fail if the exception is propagated: wrap store.close() in a try/catch where a caught exception triggers Assert.fail (or fail()) to assert no propagation, and after the try/catch add verify(mockDbSource).closeDB() to ensure the throwing path was exercised; reference the existing mockDbSource and originalDbSource variables and the CheckPointV2Store.store.close() invocation when making these additions.
52-128: Duplicated reflection boilerplate + same null-guard caveat asTronDatabaseTest.Two nits across the four tests:
- The
dbSourceField/writeOptionsFieldreflection-and-replace dance is repeated in every test. Extracting a small static helper (e.g.,replaceField(Object target, Class<?> owner, String name, Object newValue)returning the original value) would meaningfully shrink each test and make intent clearer.- Each
finallycallsoriginalDbSource.closeDB()unconditionally (lines 67, 85, 126). As noted onTronDatabaseTest, if the engine config ever yields a nulldbSource, the finally block NPEs and masks the real failure — a null-guard is inexpensive.Both are non-blocking; feel free to defer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/core/db/CheckPointV2StoreTest.java` around lines 52 - 128, Tests duplicate reflection boilerplate and call originalDbSource.closeDB() without a null-guard; create a small static helper method (e.g., replaceField(Object target, Class<?> owner, String name, Object newValue) that uses reflection to get the declared Field, setAccessible(true), swap in newValue and return the original value) and use it to replace usages of dbSourceField/writeOptionsField in CheckPointV2StoreTest and the related tests, and update the finally blocks to check that the returned originalDbSource (and any original writeOptions) is non-null before calling closeDB()/close() to avoid NPEs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/src/test/java/org/tron/core/db/TronDatabaseTest.java`:
- Around line 140-152: The test can NPE in the finally block if originalDbSource
is null; update the finally to null-check originalDbSource before calling
originalDbSource.closeDB() (i.e., only call closeDB on originalDbSource if
originalDbSource != null) so the test teardown won't mask the real failure when
dbSource wasn't initialized by TronDatabase; locate the variable
originalDbSource and the finally block in the test and add the guard around the
closeDB call.
---
Nitpick comments:
In `@framework/src/test/java/org/tron/core/db/CheckPointV2StoreTest.java`:
- Around line 71-87: The test testCloseWhenDbSourceThrows currently lacks
explicit assertions—update it to verify that the mocked DbSourceInter's
closeDB() was invoked and to explicitly fail if the exception is propagated:
wrap store.close() in a try/catch where a caught exception triggers Assert.fail
(or fail()) to assert no propagation, and after the try/catch add
verify(mockDbSource).closeDB() to ensure the throwing path was exercised;
reference the existing mockDbSource and originalDbSource variables and the
CheckPointV2Store.store.close() invocation when making these additions.
- Around line 52-128: Tests duplicate reflection boilerplate and call
originalDbSource.closeDB() without a null-guard; create a small static helper
method (e.g., replaceField(Object target, Class<?> owner, String name, Object
newValue) that uses reflection to get the declared Field, setAccessible(true),
swap in newValue and return the original value) and use it to replace usages of
dbSourceField/writeOptionsField in CheckPointV2StoreTest and the related tests,
and update the finally blocks to check that the returned originalDbSource (and
any original writeOptions) is non-null before calling closeDB()/close() to avoid
NPEs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c791f9e6-9420-4b32-b600-3c99df3c9ad3
📒 Files selected for processing (4)
chainbase/src/main/java/org/tron/core/db/TronDatabase.javachainbase/src/main/java/org/tron/core/store/CheckPointV2Store.javaframework/src/test/java/org/tron/core/db/CheckPointV2StoreTest.javaframework/src/test/java/org/tron/core/db/TronDatabaseTest.java
Comment on lines
+140
to
+152
| Field dbSourceField = TronDatabase.class.getDeclaredField("dbSource"); | ||
| dbSourceField.setAccessible(true); | ||
| DbSourceInter<byte[]> originalDbSource = (DbSourceInter<byte[]>) dbSourceField.get(db); | ||
| DbSourceInter<byte[]> mockDbSource = mock(DbSourceInter.class); | ||
| dbSourceField.set(db, mockDbSource); | ||
|
|
||
| try { | ||
| db.doClose(); | ||
| verify(spyWriteOptions).close(); | ||
| verify(mockDbSource).closeDB(); | ||
| } finally { | ||
| originalDbSource.closeDB(); | ||
| } |
There was a problem hiding this comment.
Minor: originalDbSource could be null if engine config is unset.
TronDatabase's constructor only assigns dbSource when the configured db engine is LEVELDB or ROCKSDB (TronDatabase.java lines 39–47); if the test config ever changes or the engine string doesn't match, line 142 returns null and the finally at line 151 NPEs, masking the real test failure. A null-guard is cheap insurance:
🛡️ Proposed defensive guard
} finally {
- originalDbSource.closeDB();
+ if (originalDbSource != null) {
+ originalDbSource.closeDB();
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Field dbSourceField = TronDatabase.class.getDeclaredField("dbSource"); | |
| dbSourceField.setAccessible(true); | |
| DbSourceInter<byte[]> originalDbSource = (DbSourceInter<byte[]>) dbSourceField.get(db); | |
| DbSourceInter<byte[]> mockDbSource = mock(DbSourceInter.class); | |
| dbSourceField.set(db, mockDbSource); | |
| try { | |
| db.doClose(); | |
| verify(spyWriteOptions).close(); | |
| verify(mockDbSource).closeDB(); | |
| } finally { | |
| originalDbSource.closeDB(); | |
| } | |
| Field dbSourceField = TronDatabase.class.getDeclaredField("dbSource"); | |
| dbSourceField.setAccessible(true); | |
| DbSourceInter<byte[]> originalDbSource = (DbSourceInter<byte[]>) dbSourceField.get(db); | |
| DbSourceInter<byte[]> mockDbSource = mock(DbSourceInter.class); | |
| dbSourceField.set(db, mockDbSource); | |
| try { | |
| db.doClose(); | |
| verify(spyWriteOptions).close(); | |
| verify(mockDbSource).closeDB(); | |
| } finally { | |
| if (originalDbSource != null) { | |
| originalDbSource.closeDB(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@framework/src/test/java/org/tron/core/db/TronDatabaseTest.java` around lines
140 - 152, The test can NPE in the finally block if originalDbSource is null;
update the finally to null-check originalDbSource before calling
originalDbSource.closeDB() (i.e., only call closeDB on originalDbSource if
originalDbSource != null) so the test teardown won't mask the real failure when
dbSource wasn't initialized by TronDatabase; locate the variable
originalDbSource and the finally block in the test and add the guard around the
closeDB call.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
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.
What does this PR do?
Why are these changes required?
This PR has been tested by:
Follow up
Extra details
Summary by cubic
Ensure database resources are always released during shutdown by restructuring close logic in
TronDatabaseandCheckPointV2Store, preventing RocksDB handle leaks and noisy logs.TronDatabase.close()into separate try-catch blocks and move cleanup into a protecteddoClose()sodbSource.closeDB()runs even ifwriteOptions.close()fails; end log is guaranteed in a finally block.CheckPointV2Store.close()to close childwriteOptions, then delegate todoClose()with debug-level logs to reduce log noise on frequent checkpoints.Written for commit e1bd17c. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests