Skip to content
Open
Show file tree
Hide file tree
Changes from 7 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
6 changes: 5 additions & 1 deletion chainbase/src/main/java/org/tron/core/db/TronDatabase.java
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,13 @@ 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.

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);
logger.warn("Failed to close dbSource in {}.", getName(), e);
} finally {
logger.info("******** End to close {}. ********", getName());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,19 +63,18 @@ public void updateByBatch(Map<byte[], byte[]> rows) {
}

/**
* close the database.
* Closes the database and releases all resources.
* This method ensures that subclass-specific resources are cleaned up
* before delegating to the parent class for complete resource cleanup.
*/
@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);
} finally {
logger.debug("******** End to close {}. ********", getName());
logger.warn("Failed to close writeOptions in {}.", getName(), e);
}
super.close();
Copy link
Copy Markdown
Collaborator

@halibobo1205 halibobo1205 Apr 17, 2026

Choose a reason for hiding this comment

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

[[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 its debug level 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());
}

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.

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.

}

}
104 changes: 104 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,104 @@
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 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