lnd: Call loader.UnloadWallet on shutdown #4591
Conversation
hsjoberg
force-pushed
the
walletloader-unload
branch
2 times, most recently
from
6849421
to
aa5db41
Compare
hsjoberg
changed the title
| @@ -381,6 +381,11 @@ func Main(cfg *Config, lisCfg ListenerCfg, shutdownChan <-chan struct{}) error { | |||
| walletInitParams = *params | |||
| privateWalletPw = walletInitParams.Password | |||
| publicWalletPw = walletInitParams.Password | |||
| defer func() { | |||
| if err := walletInitParams.Loader.UnloadWallet(); err != nil { | |||
| ltndLog.Errorf("Could not unload wallet: %v", err) | |||
There was a problem hiding this comment.
This way callers of Main would not know that there was an error with UnloadWallet() so there's no way to act (panic, or display an error message on the UI).
There was a problem hiding this comment.
You're right, any non-cli client will not get any feedback here.
This is the case for many things inside Main (and things that it calls), only the RPC calls get error feedback really.
For example, the healthchecker currently does play well with the mobile builds: https://github.com/lightningnetwork/lnd/blob/bc6e52888763765793dfa8887e7d88b517a6b913/server.go#L1268-L1313#
I suggest we take a look at mobile feedback issues at a later point. This is something I think is important and want to address, but it think it has to be designed to work with all the different errors Main can receive and breaking changes for the mobile build is most likely needed.
There was a problem hiding this comment.
Yeah, sadly many defer code paths in Main ignore returned errors. These error cases should really be refined in the near future to allow safe restarts every time. Fortunately UnloadWallet can only fail if it's not loaded which we can ignore and if the DB cannot be closed which is unlikely. So while not ideal, I think we can postpone this refactor for now.
There was a problem hiding this comment.
Agreed, I'm definitely interested in helping addressing this.
|
Now passing |
| @@ -1105,6 +1114,7 @@ func waitForWalletPassword(cfg *Config, restEndpoints []net.Addr, | |||
| RecoveryWindow: recoveryWindow, | |||
| Wallet: newWallet, | |||
| ChansToRestore: initMsg.ChanBackups, | |||
| UnloadWallet: loader.UnloadWallet, | |||
There was a problem hiding this comment.
I suspect that this will not be set if --noseedbackup is used which may eventually lead to a crash in the defer function
There was a problem hiding this comment.
Good thinking. Right now, the defer happens inside this if statement:
Line 375 in 97cfbec
I don't know how --noseedbackup works, but it's still opening a wallet? With the current code, the wallet file will not be unloaded if --noseedbackup is used.
There was a problem hiding this comment.
Yes, indeed it's inside the if statement, missed that. Won't crash.
This is required to make restart work for LndMobile builds. Not calling UnloadWallet would make `UnlockWallet` stall forever as the file is already opened.
This is required to make restart work for LndMobile builds.
Not calling
UnloadWalletwould makeUnlockWalletstall forever on the second run as the file is already opened.This was pretty much a non-issue for the normal UNIX daemon build, as sending
stopDaemonorSIGINTwould close down the process and Go's runtime would presumably handle any open files.But on mobile builds, this is not the case, thus we have to explicitly close it down.
Right now I'm just grabbing the loader all the way to
Mainanddefer-ing the call so that it will execute once we get ashutdownChansignal, I'm unsure if this is the best approach.N.B: wallet has to be opened for the main RPC server to function properly.
This is a part of an on-going work to address mobile restarting problems #4492.