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.

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

[mobile] make Ready signal for custom listeners #3775

Merged
merged 2 commits into from
Dec 17, 2019

Conversation

halseth
Copy link
Contributor

@halseth halseth commented Nov 29, 2019

This allows the caller to know when lnd is ready to accept RPC calls,
which is important for mobile applications where everything happens in
process.

Fixes #3614

This allows the caller to know when lnd is ready to accept RPC calls,
which is inmportant for mobile applications where eveything happens in
process.
Use the custom listeners' Ready channels to provide callbacks to the
mobile application when the gRPC services are ready.

NOTE: this changes the Start() API by adding one extra callback.
callback.OnResponse([]byte("started"))
// Finally we start two go routines that will call the provided
// callbacks when the RPC servers are ready to accept calls.
go func() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe a simpler approach could be to modify lnd.Main() by adding a started channel as an incoming argument which then could be used to signal that all initialization is finished and we're waiting on <-signal.ShutdownChannel(). WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because the unlocker is conditional (it's only started if the wallet is actually locked) you need to know what the state of the individual servers is to be able to react. So there would need to be two started channels which basically comes down to the implemented solution.
I think this is probably the most simple you can get with the current architecture.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, thanks for the clarificaton @guggero .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@halseth halseth requested a review from guggero December 5, 2019 19:45
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Nice and simple change! This makes it much easier to embed lnd, not only on mobile 👍

net.Listener

// Ready will be closed by the server listening on Listener.
Ready chan struct{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we make this <-chan struct{} to make sure we only ever read from it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't really, since we are reading from it one place, and closing it the other place.

callback.OnResponse([]byte("started"))
// Finally we start two go routines that will call the provided
// callbacks when the RPC servers are ready to accept calls.
go func() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because the unlocker is conditional (it's only started if the wallet is actually locked) you need to know what the state of the individual servers is to be able to react. So there would need to be two started channels which basically comes down to the implemented solution.
I think this is probably the most simple you can get with the current architecture.

@bhandras bhandras self-requested a review December 6, 2019 12:36
Copy link
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mobile] Callback when grpc server is ready
3 participants