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)
ViktorT-11
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 :) |
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.
ViktorT-11
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 |
6 participants
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. :)