fix concurrent map writes #1732
Conversation
WalkthroughThe recent update introduces thread-safety to the user account management system by integrating a mutex. This ensures that concurrent access to user account operations such as adding, retrieving, and deleting account managers is controlled, preventing potential data races or inconsistencies. Changes
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? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tools/walletextension/useraccountmanager/user_account_manager.go (5 hunks)
Additional comments: 5
tools/walletextension/useraccountmanager/user_account_manager.go (5)
6-6: Added
syncpackage to support mutex synchronization.22-22: Mutex added to the struct to handle synchronization.
37-38: Mutex locking and unlocking implemented correctly using
deferto ensure the mutex is always unlocked.54-55: Mutex synchronization added to ensure thread safety during read operations.
112-113: Mutex locking and unlocking added to protect the map during delete operations.
|
|
||
| // AddAndReturnAccountManager adds new UserAccountManager if it doesn't exist and returns it, if UserAccountManager already exists for that user just return it | ||
| func (m *UserAccountManager) AddAndReturnAccountManager(userID string) *accountmanager.AccountManager { | ||
| m.mu.Lock() | ||
| defer m.mu.Unlock() | ||
| existingUserAccountManager, exists := m.userAccountManager[userID] | ||
| if exists { | ||
| return existingUserAccountManager |
There was a problem hiding this comment.
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [19-116]
Please ensure that the PR checklist, as outlined in the project's development operations documentation, is completed and confirmed.
| @@ -47,6 +51,8 @@ func (m *UserAccountManager) AddAndReturnAccountManager(userID string) *accountm | |||
| // (we are not loading all of them at startup to limit the number of established connections) | |||
| // If a UserAccountManager does not exist for the userID, it returns nil and an error. | |||
| func (m *UserAccountManager) GetUserAccountManager(userID string) (*accountmanager.AccountManager, error) { | |||
| m.mu.Lock() | |||
There was a problem hiding this comment.
Worth using RWMutex and just doing read lock for this read op?
There was a problem hiding this comment.
Good point, using RWMutex and m.mu.RLock() here.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tools/walletextension/useraccountmanager/user_account_manager.go (5 hunks)
Additional comments: 4
tools/walletextension/useraccountmanager/user_account_manager.go (4)
6-6: Added
syncpackage to support mutex usage.22-22: Mutex added to the struct for managing concurrent access.
54-55: Correct usage of mutex for thread-safe access in
GetUserAccountManager.112-113: Correct usage of mutex for thread-safe deletion in
DeleteUserAccountManager.
| m.mu.RLock() | ||
| defer m.mu.Unlock() |
There was a problem hiding this comment.
Mismatched lock and unlock methods; RLock() should be paired with RUnlock().
- defer m.mu.Unlock()
+ defer m.mu.RUnlock()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.
| m.mu.RLock() | |
| defer m.mu.Unlock() | |
| m.mu.RLock() | |
| defer m.mu.RUnlock() |
There was a problem hiding this comment.
@zkokelj Great, thank you for addressing the lock pairing issue.
🎉
New Learnings Added
User: zkokelj
PR: ten-protocol/go-ten#1732
File: tools/walletextension/useraccountmanager/user_account_manager.go:0-0
Timestamp: 2024-01-03T11:07:08.884Z
Learning: The `AddAndReturnAccountManager` method in `user_account_manager.go` should use `RLock()` and `RUnlock()` for read-only operations to ensure proper lock pairing.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tools/walletextension/useraccountmanager/user_account_manager.go (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- tools/walletextension/useraccountmanager/user_account_manager.go
Why this change is needed
On December 31th Ten Gatewat crashed:
https://app.datadoghq.eu/logs?query=service%3Aobscuro_gateway_sepolia_testnet&cols=host%2Cservice&event=AgAAAYzAOYx5aCAipAAAAAAAAAAYAAAAAEFZekFPWXlYQUFEbFpRQ1pSdFhYZWdBQQAAACQAAAAAMDE4Y2MwOWEtN2VmYy00MWZkLWFiMjgtMjZmNjMxZDIwYWE1&index=%2A&messageDisplay=inline&refresh_mode=paused&stream_sort=time%2Casc&viz=stream&from_ts=1704027600000&to_ts=1704032100000&live=false
What changes were made as part of this PR
Add mutex to avoid concurrent writes to the map
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks
https://github.com/ten-protocol/ten-test/actions/runs/7397025563