MultiWallet struct should be tread safe#252
MultiWallet struct should be tread safe#252LasTshaMAN wants to merge 6 commits intoplanetdecred:masterfrom
Conversation
|
@itswisdomagain please could you take a look this |
itswisdomagain
left a comment
itswisdomagain
left a comment
There was a problem hiding this comment.
Will review again after this. As a rule of thumb, if an operation won't take long, don't make a copy of the wallets map or lock and unlock the mutex more than once.
multiwallet.go
Outdated
| mw.badWallets[wallet.ID] = wallet | ||
| mw.badWalletsUpdate(wallet.ID, wallet) | ||
| log.Warnf("Ignored wallet load error for wallet %d (%s)", wallet.ID, wallet.Name) | ||
| } else { | ||
| mw.wallets[wallet.ID] = wallet | ||
| mw.walletsUpdate(wallet.ID, wallet) |
There was a problem hiding this comment.
I think it's safe to updates the maps directly here without locking the mutex as they won't be accessed concurrently during this initialization. Alternatively, you can lock the mutexes before the for loop and unlock after, rather than locking/unlocking for each insert.
multiwallet.go
Outdated
| mw.CancelSync() | ||
|
|
||
| for _, wallet := range mw.wallets { | ||
| for _, wallet := range mw.walletsReadCopy() { |
There was a problem hiding this comment.
During shutdown, don't use a copy of the wallets. Lock the mutexes here and access the mw.wallets map directly. Basically, any other operation that needs access to the map should wait for the shutting down to complete. Additionally, before unlocking the mutex, clear the map.
multiwallet.go
Outdated
| } | ||
|
|
||
| for _, wallet := range mw.wallets { | ||
| for _, wallet := range mw.walletsReadCopy() { |
There was a problem hiding this comment.
Comment above applies here. Keep the mutex locked and prevent any other access to the map until the wallets are all opened.
multiwallet.go
Outdated
|
|
||
| // walletsReadCopy is concurrently-safe way to read from mw.wallets map. | ||
| func (mw *MultiWallet) walletsReadCopy() map[int]*Wallet { | ||
| result := make(map[int]*Wallet, len(mw.wallets)) |
There was a problem hiding this comment.
Lock before reading len(mw.wallets). Also applies to the badWalletsReadyCopy method.
|
yeah, like I've mentioned above my reasoning was "to find and fix the issue in all places, without missing any, and then we can optimize on top of that"
I'll address each comment soon |
|
Cleaned up a bit and addressed all the comments above. |
|
@itswisdomagain pls take a look when you can ^ |
LasTshaMAN
force-pushed
the
multi-wallet-struct-should-be-tread-safe
branch
from
9a4e59d to
3a5d4ce
Compare
3 participants
(likely) Resolves #865
This PR adds a mutexes around
MultiWallet.walletsandMultiWallet.badWalletsmaps to prevent races during concurrent access.Haven't tested these changes myself, though.