wrap breez services with replaceable variant #1112
Conversation
JssDWt
force-pushed
the
jssdwt-detect-hibernation
branch
from
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.
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.
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.
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.
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.
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.
JssDWt
force-pushed
the
jssdwt-detect-hibernation
branch
from
fabc1a7
to
f380049
Compare
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_hibernationthat does the disconnecting/reconnecting