lnd: allow shutdown signal during IsSynced check
#9137
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
lnd.go
Outdated
| Err error | ||
| } | ||
|
|
||
| syncCheckLoop: |
There was a problem hiding this comment.
nit: would personally prefer not using syncCheckLoop and just do break
There was a problem hiding this comment.
Thanks! The reason for the labelled loop is that just using a break would've just cancelled the select statement and not the outer loop.
I refactored this a bit though so that just canceling the select statement works fine :)! I also prefer the new version without the syncCheckLoop label, as it makes it a bit clearer what the actual loop is for.
lnd.go
Outdated
| // If we're not yet synced, we'll wait for a second before | ||
| // checking again. | ||
| select { | ||
| case <-interceptor.ShutdownChannel(): |
There was a problem hiding this comment.
looks like we don't need to catch it again here since after 1s it will go to the next iteration.
There was a problem hiding this comment.
I still included this change, as we'll else block the shutdown for 1 second while waiting for this. If you think that's an unnecessary optimisation, I'm ok with removing it though and just keep the:
<-time.After(time.Second)
ViktorTigerstrom
force-pushed
the
2024-09-sync-lock-fix
branch
from
bc66e8d
to
cb39820
Compare
|
Thanks a lot for the review @yyforyongyu 🙏! Given the Approach ack, I'll take this out of draft mode :) |
lnd.go
Outdated
| IsSynced bool | ||
| TS int64 | ||
| Err error |
There was a problem hiding this comment.
nit: do these need to be exported members?
There was a problem hiding this comment.
also suggest:
synced bool
bestBlock int64
err error
There was a problem hiding this comment.
Thanks 🙏! Went with your suggestion :). I renamed the TS to bestBlockTime though, to make it easily differentiable from the bestHeight variable above. Hope that's ok!
lnd.go
Outdated
| syncedResChan := make(chan syncResult, 1) | ||
| isSynced := false | ||
|
|
||
| for !isSynced { |
There was a problem hiding this comment.
I think we can get away without the isSynced variable. Something like:
var syncedResChan = make(chan syncResult, 1)
for {
// We check if the wallet is synced in a separate goroutine as
// the call is blocking, and we want to be able to interrupt it
// if the daemon is shutting down.
go func() {
synced, bestBlock, err := activeChainControl.Wallet.
IsSynced()
syncedResChan <- syncResult{synced, bestBlock, err}
}()
select {
case <-interceptor.ShutdownChannel():
return nil
case res := <-syncedResChan:
if res.err != nil {
return mkErr("unable to determine if wallet "+
"is synced: %v", res.err)
}
if res.synced {
break
}
// If we're not yet synced, we'll wait for a second
// before checking again.
select {
case <-interceptor.ShutdownChannel():
return nil
case <-time.After(time.Second):
continue
}
}
break
}
There was a problem hiding this comment.
Thanks! Updated the PR to your suggestion 🚀. I'm in sight favour of the isSynced variable approach though as I think that's a bit more readable, but have no strong opinion :). I did however keep the "Syncing to block timestamp" log line, as I do think that adds value for users.
There was a problem hiding this comment.
I did however keep the "Syncing to block timestamp" log line, as I do think that adds value for users.
yup - was just posting pseudo code to show the design, not an exact "pls copy as is" snippet
Prior to this commit, lnd could become unresponsive to shutdown signals during the `IsSynced` check. Since the `IsSynced` check can occasionally take a long time to complete, this could result in lnd failing to shut down promptly.
ViktorTigerstrom
force-pushed
the
2024-09-sync-lock-fix
branch
from
cb39820
to
52b3a06
Compare
|
Thanks a lot for the review @ellemouton 🙏! I've addressed your feedback :) |
|
@yyforyongyu: review reminder |
Fixes #9109
Before this PR, lnd could become unresponsive to shutdown signals during the
IsSyncedcheck, which can sometimes take a long time to complete. This caused delays in shutting down lnd.I'm pushing this as a draft PR for now, as there are several possible solutions to consider:
Cancelable context: The cleanest approach might be to pass a cancelable context into the
IsSyncedfunction, allowing it to exit immediately when a shutdown signal is received. However, this would require updates to both thebtcwalletandbtcdrepositories, and we would need to wait for new releases with these changes. There are also twoactiveChainControl.ChainIO.GetBestBlock()calls inlnd.go—just before and after this PR’s changes—which could block as well, though we haven't seen extended delays in those calls.Listening to async channels: Another option is to modify the
IsSyncedfunction to listen to the underlying async channels fromGetBestBlockandGetBlockHeader. This would also require changes to thebtcwalletrepo and a release.To reviewers: I would appreciate feedback on which approach you think is best. :)