fix: add test case to cover data race between batch.Write() and importer.Commit()
#1050
Conversation
WalkthroughThis pull request introduces a new test function, Changes
Sequence Diagram(s)sequenceDiagram
participant T as TestImporterDataIntegrity
participant M1 as Initial MutableTree
participant I as ImmutableTree
participant DB as Temporary DB (LevelDB)
participant M2 as New MutableTree
T->>M1: Initialize tree & write > max batch entries
M1->>T: Save version and retrieve immutable tree
T->>I: Export nodes from immutable tree
T->>DB: Create temporary LevelDB instance
T->>M2: Instantiate new mutable tree with DB
M2->>T: Import version and add exported nodes
T->>M2: Commit changes and save new version
T->>DB: Reopen DB and verify data integrity
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
import.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (go)
- GitHub Check: Test
- GitHub Check: Benchmarks
🔇 Additional comments (1)
import.go (1)
85-103: LGTM! Improved error handling and synchronous batch operations.The changes enhance reliability by:
- Immediately checking and propagating errors from previous batch operations
- Using
WriteSync()to ensure synchronous completion- Properly handling errors from both
WriteSync()andClose()operationsThis aligns well with the PR objective of ensuring proper wait for batch operations.
beer-1
force-pushed
the
fix/importer-close
branch
from
8fafe57 to
046ba12
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
import_test.go(2 hunks)
🔇 Additional comments (5)
import_test.go (5)
3-10: Appropriate import additions for the new test case.The added imports for
encoding/binaryandgolang.org/x/crypto/sha3are correctly included to support the new test function. These packages are used for generating deterministic keys in the test.
312-326: Good test design for database persistence.Using a persistent LevelDB database instead of an in-memory database is the right approach for testing the issue described in the PR objectives. This effectively tests whether the importer correctly waits for batch operations to complete before proceeding.
Consider adding a comment explaining the specific issue being tested to make the purpose clearer:
+ // Use a persistent database to test that batch operations complete correctly + // This specifically tests that the importer waits for batch.Close() to complete tempDir := t.TempDir() db, err := dbm.NewGoLevelDB("importer-test", tempDir)
327-332: Properly closing resources in sequence.The code correctly saves the version and explicitly closes both the tree and database in the proper sequence. This is good practice and essential for testing the described issue.
334-348: Comprehensive data verification after database reopening.The test properly reopens the database and verifies that all imported data is present and correct. This is an effective way to ensure that the importer waited for all operations to complete before closing.
277-349: This test effectively addresses the PR's main objective.This test function comprehensively tests the data integrity issue described in the PR objectives. By writing more than the maximum batch size, using a persistent database, and verifying data after database reopening, it correctly tests whether the importer waits for batch operations to complete.
The test aligns perfectly with the PR description: "fix: importer to wait db close" by ensuring database operations are properly completed before proceeding.
beer-1
changed the title
batch.Write() and importer.Commit()
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
import_test.go (3)
303-308: Inconsistent error handling pattern forErrorExportDone.The current error check uses direct equality comparison
err == ErrorExportDone, whereas other tests in this file useerrors.Is(err, ErrorExportDone). Consider usingerrors.Is()for consistency and better error handling with wrapped errors.- if err == ErrorExportDone { + if errors.Is(err, ErrorExportDone) { break } - - require.NoError(t, err) + else if err != nil { + require.NoError(t, err) + }
311-331: Ensure proper resource management with defer statements.While the test correctly closes resources, consider using
deferfor consistent resource cleanup, especially for the importer and database connections. This ensures resources are properly released even if the test fails partway through.importer, err := newTree.Import(version) require.NoError(t, err) + defer importer.Close() for _, node := range exported { err = importer.Add(node) require.NoError(t, err) } err = importer.Commit() require.NoError(t, err) - importer.Close() _, version, err = newTree.SaveVersion() require.NoError(t, err) + defer func() { + err = newTree.Close() + require.NoError(t, err) + err = db.Close() + require.NoError(t, err) + }() - err = newTree.Close() - require.NoError(t, err) - err = db.Close() - require.NoError(t, err)
284-286: Consider parameterizing the test data generation.For better maintainability, consider extracting the key and value generation logic into a helper function, especially since this pattern is repeated later in the test for verification.
+ generateKeyValue := func(i int) ([]byte, []byte) { + key := sha3.Sum256(binary.BigEndian.AppendUint64([]byte{}, uint64(i))) + return key[:], []byte{byte(i)} + } + // write more than maxBatchSize for i := 0; i < maxBatchSize+1; i++ { - bz := sha3.Sum256(binary.BigEndian.AppendUint64([]byte{}, uint64(i))) - _, err := tree.Set(bz[:], []byte{byte(i)}) + key, value := generateKeyValue(i) + _, err := tree.Set(key, value) require.NoError(t, err) }And then reuse it during verification:
for i := 0; i < maxBatchSize+1; i++ { - bz := sha3.Sum256(binary.BigEndian.AppendUint64([]byte{}, uint64(i))) - value, err := itree.Get(bz[:]) + key, expectedValue := generateKeyValue(i) + value, err := itree.Get(key) require.NoError(t, err) - require.Equal(t, []byte{byte(i)}, value) + require.Equal(t, expectedValue, value) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
import_test.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Benchmarks
- GitHub Check: Test
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
import_test.go (2)
4-10: Import statements added appropriately for the new test.The new imports for binary encoding and SHA3 hashing are correctly added to support the key generation mechanism in
TestImporterDataIntegrity.
278-348: Well-structured test that effectively validates data integrity across batches.This test effectively validates the fix for the data race between
batch.Write()andimporter.Commit()by:
- Creating a tree with entries exceeding
maxBatchSize- Exporting and then importing the data into a new LevelDB-backed tree
- Closing and reopening the database to verify persistence
- Checking that all values were correctly saved
The test properly uses temporary directories, handles resources with appropriate cleanup, and employs thorough error checking. This is an excellent test case that specifically targets the race condition mentioned in the PR objectives.
|
@beer-1 lgtm when we fix the lints (just can add //nolint comment) |
Here is context: cosmos/cosmos-sdk#23740
When we use statesync, it is using this IAVL importer but sometimes the IAVL store is empty even there is no notable errors. And after digging the issue, I found importer is not waiting the
batch.Close().This PR is to fix to importer to wait previous batch's
WriteSync()andClose().Additional context after writing the testcode
Hey @aljo242
I found
(cosmos-db)Batch.Write()is already handling this issue, by callingbatch.Commit()here atbatch.Write().So this was not the cause, but it is only happened in
v1.2.xandv1.3.x.This commit should be back ported to
v1.2.xandv1.3.x.in the meantime, I have changed this PR to add test case to cover data race between
batch.Write()andimporter.Commit(). If you run this test atv1.2.xorv1.3.xbranch, then it will be failed.Here is the PR for
release/v1.2.x#1057.