Background Crypto store usage #4370
Conversation
| @@ -226,7 +226,7 @@ internal class DefaultCryptoService @Inject constructor( | |||
| override fun fetchDevicesList(callback: MatrixCallback<DevicesListResponse>) { | |||
| getDevicesTask | |||
| .configureWith { | |||
| // this.executionThread = TaskThread.CRYPTO | |||
| this.callbackThread = TaskThread.CRYPTO | |||
There was a problem hiding this comment.
by default the executor switches back to the main thread for the callback
There was a problem hiding this comment.
I wish #2449 get fully implemented one day...
| @@ -226,7 +226,7 @@ internal class DefaultCryptoService @Inject constructor( | |||
| override fun fetchDevicesList(callback: MatrixCallback<DevicesListResponse>) { | |||
| getDevicesTask | |||
| .configureWith { | |||
| // this.executionThread = TaskThread.CRYPTO | |||
| this.callbackThread = TaskThread.CRYPTO | |||
There was a problem hiding this comment.
I wish #2449 get fully implemented one day...
| private val lazyRealmConfiguration by lazy { | ||
| Log.e("!!!", "ensureCryptoMetadataEntity ${Thread.currentThread().name}") | ||
| ensureCryptoMetadataEntity(realmConfiguration) | ||
| realmConfiguration |
There was a problem hiding this comment.
How can we ensure that in the future realmConfiguration is not used directly by mistake? Maybe first add a comment?
There was a problem hiding this comment.
hmmmmm we could create a LazyRealmConfiguration abstraction, what do you think?
also happy to add a comment (and remove the Log)
There was a problem hiding this comment.
Maybe a comment on realmConfiguration to warn developer is enough... This code will be removed in a near future (Crypto Rust SDK).
And yes, please remove the log 👍
| observeInitialSync() | ||
| checkSessionPushIsOn() | ||
| observeCrossSigningReset() | ||
| override fun handle(action: HomeActivityViewActions) { |
There was a problem hiding this comment.
The diff is a bit strange here, at least from GitHub POW.
The method handle was already existing.
Also forks may suffer from this change, so it's probably better to ensure that they are minimal.
There was a problem hiding this comment.
the diff looks really painful but all it's doing is moving the handle function to the top of the file and inlining the init functions into the ViewStarted action
I'll revert and leave the functions above the handle to improve the diff
There was a problem hiding this comment.
updated including the started guard e354313
| when (action) { | ||
| HomeActivityViewActions.PushPromptHasBeenReviewed -> vectorPreferences.setDidAskUserToEnableSessionPush() | ||
| HomeActivityViewActions.ViewStarted -> { | ||
| cleanupFiles() |
There was a problem hiding this comment.
Previously it was done only once, now it's done every time the Activity is created, i.e. also after a device rotation. Use isStarted pattern like in UnknownDeviceDetectorSharedViewModel?
There was a problem hiding this comment.
good point, I'll guard all of the calls with isStarted to avoid any behaviour changes
ouchadam
force-pushed
the
feature/adm/lazy-crypto-init
branch
3 times, most recently
from
6afa91e to
240ae9a
Compare
| } | ||
| isStarted = true | ||
|
|
||
| viewModelScope.launch { |
There was a problem hiding this comment.
the diff looks big but the only change (apart from delaying the calls to startObserving) is moving the init logic into this viewModelScope.launch
|
having been running this locally for 7+ days with no issues, promoting from draft |
|
@ouchadam ktlint is not happy, there is a useless import to remove Then I think we can merge it! |
…ialise - ensuring the crypto metadata is inserted doesn't take too long but the Realm.getInstance can be very heavy so we should delay where possible
…ocking work during object construction (init)
- also lazily loads myDevice
…itialisation tasks _after_ the Activity has started observing the events - fixing any missing events which trigger before init and onCreate - moves the cross sign off of the main thread
…han init - also access the crypto devices off of the main thread
…alm crypto store on the main thread during the dagger graph creation
…g saveMyDevicesInfo on the main thread
…y in the realm crypto store as we have the lazy version
ouchadam
force-pushed
the
feature/adm/lazy-crypto-init
branch
from
0865710 to
928f62e
Compare
|
whilst rebasing I've found an issue with signout, am investigating edit: the issue is present in develop #4480 |
ouchadam
force-pushed
the
feature/adm/lazy-crypto-init
branch
from
7d2d5cd to
928f62e
Compare
|
going to close, the crypto is being reworked with the rust SDK changes |
Fixes some parts of the crypto store flow being on the main thread, this will help address a bunch of
RealmCryptoStorerelated ANRs as we'll no longer be callingRealm.getInstanceon the main thread when interacting with the crypto store.initlogicinitlogic toStarteventsThis could break all sorts of things, needs a bunch more testing!
Before - Sign in
After - Sign in