Close wallet db on shutdown #566
Conversation
|
Hi, would you support SegWit and LND? |
| // some reason, however, the wallet will not be marked synced | ||
| // and many methods will error early since the wallet is known | ||
| // to be out of date. | ||
| defer w.wg.Done() |
There was a problem hiding this comment.
style nit: move this to the top of the method.
wallet/wallet.go
Outdated
|
|
||
| //We can close the db now that we know all go routines have exited and | ||
| //we are sure no further attempts to write to the wallet db will be done. | ||
| w.db.Close() |
There was a problem hiding this comment.
Nice catch!
However, I don't think this is the correct place to close the db. Ideally it should be closed from the same context as where it is opened. Since the db is opened by the wallet loader, we should instead make sure UnloadWallet is called properly from the loader.
It looks like UnloadWallet waits for the wallet to shutdown before closing the db, so maybe just adding the onClientConnected goroutine is enough?
There was a problem hiding this comment.
Thanks for your review @halseth and sorry for my late response, somehow lost track of this issue.
To see that I understand:
- I will leave the onClientConnected function since it adds a waiting for the syncToChain go routine that is needed.
- I will remove the db.Close call.
The impact of this is that users that uses btcwallet as library will be responsible to call loader.UnloadWallet on shutdown and not only wallet.Stop (as it is now in LND).
If this is what you think needed I will need to create another fix in LND to explicitly call UnloadWallet on shutdown.
LMK your thoughts.
There was a problem hiding this comment.
At the moment I removed the db.Close and rebased.
roeierez
force-pushed
the
gracefull_shutdown
branch
from
a1da503 to
2856c18
Compare
There was a problem hiding this comment.
Yes, I think that's correct. In lnd's case that means that it should be unloaded from this context: https://github.com/lightningnetwork/lnd/blob/d6d833c2421f4a65c6db5190d1dcd73f549fbf70/lnwallet/btcwallet/btcwallet.go#L116. Haven't looked into where exactly it should be called from though.
| @@ -317,6 +317,22 @@ func (w *Wallet) activeData(dbtx walletdb.ReadTx) ([]btcutil.Address, []wtxmgr.C | |||
| return addrs, unspent, err | |||
| } | |||
|
|
|||
| func (w *Wallet) onClientConnected(birthdayStamp *waddrmgr.BlockStamp) { | |||
| w.wg.Add(1) | |||
There was a problem hiding this comment.
this should be called the line before the goroutine
There was a problem hiding this comment.
@halseth another issue with the syncWithChain function related to the shutdown is that it runs in a loop and doesn't check for ShuttingDown() during its run. The problem even gets worst when the shutdown request comes at time where the code is at this point: https://github.com/btcsuite/btcwallet/blob/master/wallet/wallet.go#L460
the go routine keeps running forever stuck inside this block preventing from wallet shutdown.
I can add such kind of check inside this block and on every iteration of the outer loop. Do you think it is a good solution or can suggest a better one?
There was a problem hiding this comment.
Yeah, checking the quit signal on every iteration sounds like a good idea :)
roeierez
force-pushed
the
gracefull_shutdown
branch
from
0cfec82 to
92a6c73
Compare
|
Squashed. |
This PR ensure that after the wallet is shut down the database will be closed as well.
To do so the "syncWithChain" go routine was added to the wait group to make sure it competes before we attempt to close the wallet db.
In addition I added explicit db.Close call after all go routines have exited.
This is crucial to be able to use dependent systems such LND as library (such as in mobile) and run it multiple times on the same process.