-
Notifications
You must be signed in to change notification settings - Fork 45
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
wrap breez services with replaceable variant #1112
Conversation
1f0ad15
to
fabc1a7
Compare
libs/sdk-core/src/breez_services.rs
Outdated
} | ||
/// Force running backup | ||
pub async fn backup(&self) -> SdkResult<()> { | ||
let services = Arc::clone(&*self.services.lock().await); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like every BreezServices method is now holding a lock for its execution time? It means we no longer can run these methods in parallel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order not to block I think we need to create a method get_services() that returns a cloned instance (and the lock is then released at the end of that getter method).
Did you think about a more minimalistic approach such as only reconnect the grpc clients? In the greenlight node_api for example it seems very easy as setting the inner node_client to null (which will trigger a reconnection on next call): https://github.com/breez/breez-sdk-greenlight/blob/main/libs/sdk-core/src/greenlight/node_api.rs#L264
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like every BreezServices method is now holding a lock for its execution time? It means we no longer can run these methods in parallel?
The idea was this takes a lock only on that line, I'll double check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you think about a more minimalistic approach such as only reconnect the grpc clients? In the greenlight node_api for example it seems very easy as setting the inner node_client to null (which will trigger a reconnection on next call): https://github.com/breez/breez-sdk-greenlight/blob/main/libs/sdk-core/src/greenlight/node_api.rs#L264
There's also the signer/scheduler connections that will need replacing.
Replacing the grpc clients means replacing at least the background services that interact with the grpc streams. Then there's the breez server client that needs replacing too.
I started thinking about only changing the necessary things, but then this just seemed much simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like every BreezServices method is now holding a lock for its execution time? It means we no longer can run these methods in parallel?
I've moved the lock into a get_services
function now, which looks cleaner.
If an app hibernates, the breez sdk may still run. However, any connections to the outside world, like grpc connections will (may) stop functioning. This means after hibernation the sdk needs to reconnect. Hibernation is detected by awaiting a 'sleep' in a loop. If the sleep has taken a long time, that means the app has been in hibernation. Because many services have references to the node api, and other services than the greenlight client may have been affected by hibernation, the entire breezservices instance is recreated, reconnected. In order to allow this, an internal variant of breezservices was made. This internal instance can be replaced at any time. Because the breezservices code was moved to another file, with edits, I took the liberty of putting all functions inside breezservices in alphabetical order.
fabc1a7
to
f380049
Compare
Closing in favor of #1115 |
If an app hibernates, the breez sdk may still run. However, any connections to the outside world, like grpc connections will (may) stop functioning. This means after hibernation the sdk needs to reconnect. Hibernation is detected by awaiting a 'sleep' in a loop. If the sleep has taken a long time, that means the app has been in hibernation. Because many services have references to the node api, and other services than the greenlight client may have been affected by hibernation, the entire breezservices instance is recreated, reconnected. In order to allow this, an internal variant of breezservices was made. This internal instance can be replaced at any time.
Because the breezservices code was moved to another file, with edits, I took the liberty of putting all functions inside breezservices in alphabetical order.
Related: Blockstream/greenlight#521
Tested with c-breez build https://github.com/breez/c-breez/actions/runs/11519175590
I see the app hibernating and succesfully reconnecting.
TODO: The SDK is in an unrecoverable state right now if the reconnect fails.
Review remarks:
detect_hibernation
that does the disconnecting/reconnecting