Make sure the logout action doesn't cause a crash #3480
Conversation
Some reasons why this could happen: 1. The `ClientDelegate` could receive a `didReceiveAuthError` callback call on a logout, which could trigger another logout when every Rust object had already been destroyed. 2. Even though we stop the sync before logging out, `LoggedInFlowNode` will try to start it again automatically when it detects we still have internet connection. Making sure to unregister the delegate should fix the first part of the issue. For the other one, adding `RustSyncService.isServiceReady` to check if we should start/stop the service, which is enabled by default and set to false on destroy should help.
| @@ -50,7 +50,6 @@ class RustClientSessionDelegate( | |||
| */ | |||
| fun bindClient(client: RustMatrixClient) { | |||
| this.client = client | |||
| client.setDelegate(this) | |||
There was a problem hiding this comment.
This was never really needed, as RustMatrixClient automatically sets its inner Client delegate.
|
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3480 +/- ##
========================================
Coverage 82.66% 82.66%
========================================
Files 1731 1731
Lines 40829 40829
Branches 4964 4964
========================================
Hits 33750 33750
Misses 5320 5320
Partials 1759 1759 ☔ View full report in Codecov by Sentry. |
Done in 1473afc if you want to have a quick look @jmartinesp |
|
jmartinesp
added
the
Run-Maestro
github-actions
bot
removed
the
Run-Maestro
| override suspend fun startSync() = runCatching { | ||
| if (!isServiceReady.get()) { |
There was a problem hiding this comment.
Do we really need those checks, the failure will be catched by runCatching anyway
There was a problem hiding this comment.
This way we can have more detailed logging in case we run into a similar issue in the future, but you're right, it's not really needed.
There was a problem hiding this comment.
With the runcatching we are already logging the exception if I'm correct
| Timber.i("Start sync") | ||
| innerSyncService.start() | ||
| }.onFailure { | ||
| Timber.d("Start sync failed: $it") | ||
| } | ||
|
|
||
| override suspend fun stopSync() = runCatching { | ||
| if (!isServiceReady.get()) { |
Content
Making sure to unregister the delegate should fix the first part of the issue.
For the other one, adding
RustSyncService.isServiceReadyto check if we should start/stop the service, which is enabled by default and set to false on destroy should help.We should be extra careful to also include this same behaviour to account deactivation after #3479 if this PR is merged.
Motivation and context
Some reasons why this could happen:
ClientDelegatecould receive adidReceiveAuthErrorcallback call on a logout, which could trigger another logout when every Rust object had already been destroyed.LoggedInFlowNodewill try to start it again automatically when it detects we still have internet connection.Tests
If it doesn't create a crash anymore, it's fixed. Also, failures while logging out should restore the client delegate and allow the user to keep refreshing their token and receive auth errors.
Tested devices
Checklist