fix: add mu to Dummy executor #58
Conversation
WalkthroughThe changes introduce a mutex into the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Executor as DummyExecutor
participant Mutex as RWMutex
Note over Executor: For write operations (InitChain, InjectTx, ExecuteTxs, SetFinal)
Caller->>Executor: Invoke write method
Executor->>Mutex: Lock()
Executor->>Executor: Execute critical section
Mutex-->>Executor: Unlock()
Executor->>Caller: Return result
Note over Executor: For read operations (GetTxs)
Caller->>Executor: Invoke GetTxs
Executor->>Mutex: RLock()
Executor->>Executor: Copy transactions
Mutex-->>Executor: RUnlock()
Executor->>Caller: Return copied transactions
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
RollkitBot
requested review from
a team,
tuxcanfly,
gupadhyaya and
MSevey
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/dummy.go (1)
16-22: Add concurrent access tests.While the mutex implementation is correct, consider adding tests that verify thread safety by exercising concurrent access patterns. This would help ensure the synchronization works as intended.
Would you like me to help generate concurrent test cases that verify the thread safety of these methods?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/dummy.go(4 hunks)
🔇 Additional comments (6)
test/dummy.go (6)
9-9: LGTM! Good choice using RWMutex.Using
sync.RWMutexis appropriate here as it allows multiple concurrent readers while ensuring exclusive access for writers.Also applies to: 17-17
36-38: LGTM! Proper mutex usage for state modification.The write lock ensures thread-safe modification of
stateRoot, anddeferensures the lock is always released.
47-52: LGTM! Excellent defensive programming.Great implementation with:
- Using RLock for read-only operation
- Creating a defensive copy to prevent external modifications
57-61: Verify potential deadlock scenario with removeExecutedTxs.While the locking in
InjectTxis correct, note thatremoveExecutedTxsalso modifiesinjectedTxsand is called fromExecuteTxswhile holding the lock. This is currently safe but could become a source of deadlocks ifremoveExecutedTxsis ever called from other contexts.Consider documenting that
removeExecutedTxsmust only be called while holding the mutex:+// removeExecutedTxs removes the executed transactions from injectedTxs. +// Note: This method must only be called while holding e.mu lock. func (e *DummyExecutor) removeExecutedTxs(txs []types.Tx) {Also applies to: 92-96
65-67: LGTM! Proper locking for multiple state modifications.The write lock correctly protects both
pendingRootsmodification and the call toremoveExecutedTxs.
81-83: LGTM! Proper locking for state finalization.The write lock correctly protects modifications to both
stateRootandpendingRoots.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
test/dummy.go (1)
72-85:⚠️ Potential issueProtect removeExecutedTxs with mutex lock.
The
removeExecutedTxsmethod is called while holding the lock, but it's not protected by the mutex itself. This could lead to race conditions if called directly from other methods.Move the implementation into ExecuteTxs or add mutex protection to removeExecutedTxs:
-func (e *DummyExecutor) removeExecutedTxs(txs []types.Tx) { +func (e *DummyExecutor) removeExecutedTxs(txs []types.Tx) { + e.mu.Lock() + defer e.mu.Unlock() + e.injectedTxs = slices.DeleteFunc(e.injectedTxs, func(tx types.Tx) bool { return slices.ContainsFunc(txs, func(t types.Tx) bool { return bytes.Equal(tx, t) }) }) }
🧹 Nitpick comments (1)
test/dummy.go (1)
15-22: Document concurrency guarantees.Consider adding documentation about thread safety guarantees of the DummyExecutor struct.
// DummyExecutor is a dummy implementation of the DummyExecutor interface for testing +// It is safe for concurrent use. type DummyExecutor struct {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/dummy.go(3 hunks)
🔇 Additional comments (2)
test/dummy.go (2)
9-9: LGTM! Good choice using RWMutex.The addition of
sync.RWMutexis appropriate here as it allows concurrent reads while ensuring exclusive writes.Also applies to: 17-17
33-39: LGTM! Well-implemented thread-safe getter.The implementation follows the suggested pattern with proper read locking and documentation.
yarikbratashchuk
force-pushed
the
yarik/data-race
branch
from
55eca42 to
88596fd
Compare
|
🎉 This PR is included in version 0.2.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary by CodeRabbit