mobile: Add ServiceStatus for retrieving started up status of lnd - #4595
mobile: Add ServiceStatus for retrieving started up status of lnd#4595hsjoberg wants to merge 1 commit into
Conversation
915084f to
fa9ca9b
Compare
cfromknecht
left a comment
There was a problem hiding this comment.
@hsjoberg thanks for the PR! no major comments, just one small nit about avoiding race conditions
|
|
||
| // LndStatus holds the current status of lnd. | ||
| type LndStatus struct { | ||
| // lndStarted tells whether the lnd is started or not. |
There was a problem hiding this comment.
it looks these fields could use a mutex, since it's modified and read concurrently
| } | ||
|
|
||
| func GetStatus(callback LndStatusCallback) { | ||
| callback.OnResponse(lndStatus.lndStarted, lndStatus.walletUnlocked) |
There was a problem hiding this comment.
Is the application going to poll by calling GetStatus from time to time?
There was a problem hiding this comment.
@bhandras No that's not what I had in mind -- rather call GetStatus once on app startup.
But sure an application could also possibly poll this one if needed.
|
@halseth What I'm trying to accomplish is checking the state of lnd regardless if a gRPC server is active or not. So using a gRPC server here defeats the purpose. Compared to running as a daemon/in terminal, when using mobile bindings you call |
|
@hsjoberg I see, thanks for explaining! Would an alternative solution be to make |
|
Aiming to solve something similar with #5005 |
|
I think we should revisit this. Currently, there is a chicken-or-egg problem when it comes to starting Blixt.
For these reasons, I think it makes sense to expose status in a non-grpc manner, so mobile clients and other embedded-lnd usecases can track state reliably. GRPC is unusable at this stage of the app "booting up". Also proposing a rename here of |
|
Reopening this one as there are certain situations where it's beneficial or needed to know whether is considered started up or not, without querying the gRPC API itself. In Blixt Wallet for Android, our background sync job calls Another potential scenario could be if the app needs to close down lnd, for example in order to change the configuration. |
fa9ca9b to
94a9972
Compare
|
Greatly simplified the implementation and changed the name of the function to Also, this code has been used in production in Blixt for some years now, so it is battle-tested. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a ServiceStatus function to the mobile bindings, allowing the host application to check if the lnd service has been started. This is a useful addition for mobile applications to manage the lnd lifecycle correctly.
My review includes two main points:
- A minor style guide fix for the function comment to adhere to the repository's standards.
- A more critical fix for a potential data race by ensuring an atomic read of the status variable.
After addressing these points, the change should be good to merge.
94a9972 to
90937c2
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a ServiceStatus function to allow mobile clients to query the status of the lnd service. This is a useful addition for mobile applications to handle state synchronization with the lnd process, especially in scenarios like service crashes.
My main feedback is to make the returned status more comprehensive by including the wallet's lock state, as mentioned in your pull request description. The current implementation only returns whether lnd has started. Expanding the status information would greatly increase the utility of this new API for mobile clients. I've provided a suggestion on how to structure this.
| type LndStatusCallback interface { | ||
| OnResponse(lndStarted int32) | ||
| } | ||
|
|
||
| // ServiceStatus returns 1 if lnd has started up successfully, allowing the | ||
| // caller to interact with the in-memory gRPC servers. If 0 is returned, | ||
| // the caller should invoke `Start` to initialize lnd. | ||
| // | ||
| // ServiceStatus can also be used to determine when lnd has shut down, since | ||
| // `stopDaemon` returns immediately and does not wait for the gRPC servers to be | ||
| // fully closed. | ||
| func ServiceStatus(callback LndStatusCallback) { | ||
| callback.OnResponse(atomic.LoadInt32(&lndStarted)) | ||
| } |
There was a problem hiding this comment.
Your PR description mentions that the status should include walletUnlocked, which is a great idea for mobile clients. The current implementation only returns lndStarted. To make this function more useful and align with your description, I suggest returning a structured status object.
This makes the API more extensible for future status fields. You could then use the generated gRPC bindings for the StateService to query the wallet's state when lnd is running.
Here is a suggested structure that you can build upon. Note that you'll also need to import the encoding/json package.
// LndStatus represents the current status of the lnd daemon.
type LndStatus struct {
LndStarted bool `json:"lnd_started"`
// WalletState is the current state of the wallet, matching the states
// from the `lnrpc.WalletState` enum. A value of 0 indicates that the
// state is unknown, which is the case if lnd is not running.
WalletState int `json:"wallet_state"`
}
// LndStatusCallback is an interface that must be implemented by the caller
// to retrieve the service status.
type LndStatusCallback interface {
// OnResponse is called with the status of lnd, serialized as a JSON
// byte slice. An error is returned as a string if one occurred.
OnResponse(statusBytes []byte, errStr string)
}
// ServiceStatus retrieves the current status of the lnd daemon, including
// whether it has started and the wallet's current state. The status is passed
// to the provided callback as a JSON-encoded byte slice.
func ServiceStatus(callback LndStatusCallback) {
// NOTE: This implementation only returns the lndStarted status.
// A future commit should expand this to query the wallet state via the
// State gRPC service when lnd is running.
status := &LndStatus{
LndStarted: atomic.LoadInt32(&lndStarted) == 1,
}
statusBytes, err := json.Marshal(status)
if err != nil {
callback.OnResponse(nil, "error marshalling status: "+err.Error())
return
}
callback.OnResponse(statusBytes, "")
}There was a problem hiding this comment.
I've updated the PR description now.
The old implementation had walletUnlocked bool.
But to keep the scope low and functionality targeted, I suggest checking wallet state in the gRPC server later on once lnd is running. This can be done with subscribeState. It's basically a necessity for wallet apps to subscribe to this stream anyway.
Figuring out wallet state is a solved problem. However figuring out whether lnd is started is not.
Used for checking whether lnd is started or not. ServiceStatus can also be used to determine when lnd has shut down, since `stopDaemon` returns immediately and does not wait for the gRPC servers to be fully closed.
90937c2 to
aca307f
Compare
|
@hsjoberg, remember to re-request review from reviewers when ready |
2 similar comments
|
@hsjoberg, remember to re-request review from reviewers when ready |
|
@hsjoberg, remember to re-request review from reviewers when ready |
This PR adds a new exported gomobile function
ServiceStatusthat can be used to figure out lnd's runtime state (as in whether it's considered started or not).This is helpful in certain situations, see my comments below (#4595 (comment)).
Cheers