client/core: prevent concurrent login requests #1749
Conversation
ukane-philemon
force-pushed
the
loginfix
branch
from
f81c896
to
ac0219a
Compare
| @@ -3689,6 +3713,9 @@ func (c *Core) Logout() error { | |||
| dc.acct.lock() | |||
| } | |||
|
|
|||
| c.mtx.Lock() | |||
| c.loginWg = nil | |||
There was a problem hiding this comment.
Oh I just realized this PR makes future Login calls (not just the ones that are waiting on concurrent ones to finish) skip over the wallet connect and c.resolveActiveTrades calls.
I think for 0.6 we look into a more drastic change that nukes the LoginResult so we don't even have to do initializeDEXConnections, but for 0.5 we don't break the API and return the result the caller expects.
There was a problem hiding this comment.
I'm going to propose a different, less fundamental, change for 0.5, but we can keep investigating in this PR.
The thing about the first login attempt forever blocking subsequent logins from attempting wallet connections and c.resolveActiveTrades is that if a wallet failed to connect on that initial attempt, the trackedTrades using that wallet will not be loaded into the dc.trades map, and they will have just vanished until they can run c.resolveActiveTrades again with connected wallets. Maybe we need to rethink that design, but for now I don't want to eliminate the ability to have this reattempted by calling Login again. The user can try to Logout so that they may Login again, but they'll get blocked from doing that if there are other active orders.
In matrix we were discussing a Refresh function perhaps, or putting an an override arg to Logout. Probably we should load all trades into the map, even if they don't have a wallet, but that will also take work and probably break things. So these are future changes to make.
| c.mtx.RLock() | ||
| loginWg := c.loginWg | ||
| c.mtx.RUnlock() | ||
|
|
||
| if loginWg != nil { | ||
| // User already sent a login request or is already logged in. | ||
| c.loginWg.Wait() | ||
| return loginResult(), err | ||
| } |
There was a problem hiding this comment.
c.loginWg may not be loginWg anymore when you try to wait here. It could be nil even. I think you want to do loginWg.Wait()
| c.loginWg = &sync.WaitGroup{} | ||
| c.loginWg.Add(1) | ||
| c.mtx.Unlock() | ||
| defer c.loginWg.Done() |
There was a problem hiding this comment.
I think this defer should also be called inside the lock. Although it is executed later, it is a defer statement and so evaluated immediately. https://go.dev/play/p/hLeBTmEfagk
This addresses a concern in #1690. It is a minor fix to prevent concurrent login requests.