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

Close wallet db on shutdown #566

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Close wallet db on shutdown #566

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 1 addition & 12 deletions wallet/chainntfns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,6 @@ func (w *Wallet) handleChainNotifications() {
return
}

sync := func(w *Wallet, birthdayStamp *waddrmgr.BlockStamp) {
// At the moment there is no recourse if the rescan fails for
// 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.
err := w.syncWithChain(birthdayStamp)
if err != nil && !w.ShuttingDown() {
log.Warnf("Unable to synchronize wallet to chain: %v", err)
}
}

catchUpHashes := func(w *Wallet, client chain.Interface,
height int32) error {
// TODO(aakselrod): There's a race conditon here, which
Expand Down Expand Up @@ -111,7 +100,7 @@ func (w *Wallet) handleChainNotifications() {
panic(err)
}

go sync(w, birthdayBlock)
w.onClientConnected(birthdayBlock)
case chain.BlockConnected:
err = walletdb.Update(w.db, func(tx walletdb.ReadWriteTx) error {
return w.connectBlock(tx, wtxmgr.BlockMeta(n))
Expand Down
16 changes: 16 additions & 0 deletions wallet/wallet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,22 @@ func (w *Wallet) activeData(dbtx walletdb.ReadTx) ([]btcutil.Address, []wtxmgr.C
return addrs, unspent, err
}

// onClientConnected starts calls syncWithChain in a new go routine.
// At the moment there is no recourse if the rescan fails for
// 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.
func (w *Wallet) onClientConnected(birthdayStamp *waddrmgr.BlockStamp) {
w.wg.Add(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be called the line before the goroutine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

Copy link
Contributor Author

@roeierez roeierez Dec 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, checking the quit signal on every iteration sounds like a good idea :)

go func() {
defer w.wg.Done()
Copy link
Contributor

Choose a reason for hiding this comment

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

style nit: move this to the top of the method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

err := w.syncWithChain(birthdayStamp)
if err != nil && !w.ShuttingDown() {
log.Warnf("Unable to synchronize wallet to chain: %v", err)
}
}()
}

// syncWithChain brings the wallet up to date with the current chain server
// connection. It creates a rescan request and blocks until the rescan has
// finished. The birthday block can be passed in, if set, to ensure we can
Expand Down