Skip to content
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions chainbase/src/main/java/org/tron/core/db/TronDatabase.java
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,23 @@ public void reset() {
@Override
public void close() {
logger.info("******** Begin to close {}. ********", getName());
try {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SHOULD] I think the try-finally statement is redundant; the following statement would suffice.

doClose();
logger.info("******** End to close {}. ********", getName());

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, same reasoning as the CheckPointV2Store.close() simplification from the previous round — doClose() swallows everything internally, so the outer try/finally only existed to print the End log. Applied the suggested form in dadf9bb85.

doClose();
} finally {
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 {}.", getName(), e);
} finally {
logger.info("******** End to close {}. ********", getName());
logger.warn("Failed to close dbSource in {}.", getName(), e);
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, putting the log in the second finally is a clean alternative — less nesting with the same guarantee. Looks good!


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,17 +62,16 @@ public void updateByBatch(Map<byte[], byte[]> rows) {
this.dbSource.updateByBatch(rows, writeOptions);
}

/**
* close the database.
*/
@Override
public void close() {
logger.debug("******** Begin to close {}. ********", getName());
try {
writeOptions.close();
dbSource.closeDB();
} catch (Exception e) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@warku123, [SHOULD] The fact that testCloseDbSourceWhenWriteOptionsThrows has to reflectively set BOTH CheckPointV2Store.writeOptions and TronDatabase.writeOptions proves the subclass has its own writeOptions field shadowing the parent's — two WriteOptionsWrapper instances live side by side. This is pre-existing, but the new close() structure makes the implicit contract "child closes its own writeOptions first, then delegates to parent" very fragile. Consider either promoting the parent field to protected and removing the subclass field, or adding a class-level comment explaining why two writeOptions instances are intentional.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this. The two WriteOptionsWrapper instances are intentional rather than accidental shadowing:

  • Parent TronDatabase.writeOptions is configured with isDbSync()
  • CheckPointV2Store.writeOptions is configured with isCheckpointSync() (a dedicated checkpoint-specific sync flag)

Collapsing them into a single protected field would change the sync semantics for either the parent or the checkpoint store, so it's not a zero-risk refactor. Since this shadowing pre-dates the PR and the scope here is the resource-leak fix, I'd prefer to leave the structure as-is and file a follow-up issue if you'd like the shadowing formally addressed. Happy to add a class-level comment documenting the intent if that would help.

logger.warn("Failed to close {}.", getName(), e);
logger.warn("Failed to close writeOptions in {}.", getName(), e);
}
try {
doClose();
} finally {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@warku123, [QUESTION] Why call doClose() directly instead of super.close()? I assume it's to avoid the "Begin/End to close" info logs being emitted twice, but this turns "child must run its own cleanup first and then invoke doClose() (not super.close())" into an implicit contract — if TronDatabase.close() ever grows new template logic, subclasses will silently miss it. Consider adding Javadoc on doClose() stating: "Subclasses that need to release extra resources before parent cleanup should override close(), release their own resources, and invoke doClose() directly — not super.close() — to avoid duplicated logging."

Copy link
Copy Markdown
Author

@warku123 warku123 Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point — the "call doClose() not super.close()" contract was implicit and would quietly break subclasses if TronDatabase.close() ever gained template logic. Added Javadoc on doClose() stating the contract explicitly and noting the best-effort/WARN exception semantics.

logger.debug("******** End to close {}. ********", getName());
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@warku123, [SHOULD] doClose() already swallows every exception internally (both the writeOptions and dbSource paths turn into logger.warn), so this outer try { doClose(); } finally { ... } never actually protects anything — the finally only exists to print the End log. It can be simplified to:

doClose();
logger.debug("******** End to close {}. ********", getName());

Keeping the current form isn't wrong, but the finally adds cognitive load without guarding a resource.

Copy link
Copy Markdown
Author

@warku123 warku123 Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — doClose() swallows everything internally so the outer try { ... } finally { ... } only existed to print the End log. Simplified to a straight doClose(); logger.debug(...) .

}
Expand Down
111 changes: 111 additions & 0 deletions framework/src/test/java/org/tron/core/db/CheckPointV2StoreTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
package org.tron.core.db;

import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.verify;

import java.io.IOException;
import java.lang.reflect.Field;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.ClassRule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import org.rocksdb.RocksDB;
import org.tron.common.TestConstants;
import org.tron.common.storage.WriteOptionsWrapper;
import org.tron.core.config.args.Args;
import org.tron.core.db.common.DbSourceInter;
import org.tron.core.store.CheckPointV2Store;

public class CheckPointV2StoreTest {

@ClassRule
public static final TemporaryFolder temporaryFolder = new TemporaryFolder();

static {
RocksDB.loadLibrary();
}

@BeforeClass
public static void initArgs() throws IOException {
Args.setParam(
new String[]{"-d", temporaryFolder.newFolder().toString()},
TestConstants.TEST_CONF
);
}

@AfterClass
public static void destroy() {
Args.clearParam();
}

@Test
public void testCloseWithRealResources() {
CheckPointV2Store store = new CheckPointV2Store("test-close-real");
// Exercises the real writeOptions.close() and dbSource.closeDB() code paths
store.close();
}

@Test
public void testCloseReleasesAllResources() throws Exception {
CheckPointV2Store store = new CheckPointV2Store("test-close");

// Replace dbSource with a mock so we can verify closeDB()
Field dbSourceField = TronDatabase.class.getDeclaredField("dbSource");
dbSourceField.setAccessible(true);
DbSourceInter<byte[]> originalDbSource = (DbSourceInter<byte[]>) dbSourceField.get(store);
DbSourceInter<byte[]> mockDbSource = mock(DbSourceInter.class);
dbSourceField.set(store, mockDbSource);

try {
store.close();

verify(mockDbSource).closeDB();
} finally {
originalDbSource.closeDB();
}
}

@Test
public void testCloseDbSourceWhenWriteOptionsThrows() throws Exception {
CheckPointV2Store store = new CheckPointV2Store("test-close-exception");

// Replace child writeOptions with a spy that throws on close
Field childWriteOptionsField = CheckPointV2Store.class.getDeclaredField("writeOptions");
childWriteOptionsField.setAccessible(true);
WriteOptionsWrapper childWriteOptions =
(WriteOptionsWrapper) childWriteOptionsField.get(store);
WriteOptionsWrapper spyChildWriteOptions = spy(childWriteOptions);
doThrow(new RuntimeException("simulated writeOptions failure"))
.when(spyChildWriteOptions).close();
childWriteOptionsField.set(store, spyChildWriteOptions);

// Replace parent writeOptions with a spy that throws on close
Field parentWriteOptionsField = TronDatabase.class.getDeclaredField("writeOptions");
parentWriteOptionsField.setAccessible(true);
WriteOptionsWrapper parentWriteOptions =
(WriteOptionsWrapper) parentWriteOptionsField.get(store);
WriteOptionsWrapper spyParentWriteOptions = spy(parentWriteOptions);
doThrow(new RuntimeException("simulated parent writeOptions failure"))
.when(spyParentWriteOptions).close();
parentWriteOptionsField.set(store, spyParentWriteOptions);

// Replace dbSource with a mock
Field dbSourceField = TronDatabase.class.getDeclaredField("dbSource");
dbSourceField.setAccessible(true);
DbSourceInter<byte[]> originalDbSource = (DbSourceInter<byte[]>) dbSourceField.get(store);
DbSourceInter<byte[]> mockDbSource = mock(DbSourceInter.class);
dbSourceField.set(store, mockDbSource);

try {
store.close();

// dbSource.closeDB() must be called even though both writeOptions threw
verify(mockDbSource).closeDB();
} finally {
originalDbSource.closeDB();
}
}
}
Loading