Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix concurrent map writes #1732

Merged
merged 3 commits into from
Jan 3, 2024
Merged

fix concurrent map writes #1732

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9347e1c
fix concurrent map writes
zkokelj Jan 3, 2024
1707cd1
use RWMutex
zkokelj Jan 3, 2024
99bb416
RUnlock fix
zkokelj Jan 3, 2024
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
8 changes: 8 additions & 0 deletions tools/walletextension/useraccountmanager/user_account_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package useraccountmanager
import (
"encoding/hex"
"fmt"
"sync"

"github.com/ethereum/go-ethereum/common"
gethlog "github.com/ethereum/go-ethereum/log"
Expand All @@ -18,6 +19,7 @@ type UserAccountManager struct {
storage storage.Storage
hostRPCBinAddr string
logger gethlog.Logger
mu sync.Mutex
}

func NewUserAccountManager(unauthenticatedClient rpc.Client, logger gethlog.Logger, storage storage.Storage, hostRPCBindAddr string) UserAccountManager {
Expand All @@ -32,6 +34,8 @@ func NewUserAccountManager(unauthenticatedClient rpc.Client, logger gethlog.Logg

// 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
Copy link

Choose a reason for hiding this comment

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

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.

Expand All @@ -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()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Worth using RWMutex and just doing read lock for this read op?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, using RWMutex and m.mu.RLock() here.

defer m.mu.Unlock()
userAccManager, exists := m.userAccountManager[userID]
if !exists {
return nil, fmt.Errorf("UserAccountManager doesn't exist for user: %s", userID)
Expand Down Expand Up @@ -103,6 +109,8 @@ func (m *UserAccountManager) GetUserAccountManager(userID string) (*accountmanag
// DeleteUserAccountManager removes the UserAccountManager associated with the given userID.
// It returns an error if no UserAccountManager exists for that userID.
func (m *UserAccountManager) DeleteUserAccountManager(userID string) error {
m.mu.Lock()
defer m.mu.Unlock()
_, exists := m.userAccountManager[userID]
if !exists {
return fmt.Errorf("no UserAccountManager exists for userID %s", userID)
Expand Down
Loading