New CryptoMachine on each background operation #1704
Conversation
Anderas
force-pushed
the
andy/background_crypto
branch
from
5996fb7
to
ec27724
Compare
Anderas
requested review from
a team and
nimau
and removed request for
a team
Codecov ReportBase: 17.40% // Head: 17.40% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #1704 +/- ##
========================================
Coverage 17.40% 17.40%
========================================
Files 610 610
Lines 95430 95428 -2
Branches 40200 40198 -2
========================================
Hits 16613 16613
+ Misses 78311 78309 -2
Partials 506 506
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
Hi @Anderas, I have just one question about throwing the error. Since I'm not very familiar with the CryptoMachine and what the side effects of creating a new one each time might be, it might be a good idea to request a second reviewer. |
No worries, I added one more reviewer. The cost of creating a new machine each time is mostly performance, it will obviously take a bit longer, though given the rate of notification delivery is not rapid, this is relatively minor cost. Even a small cost is not ideal though and we will eventually fix the underlying problem directly in rust-sdk (by ensuring there is no stale cache). My current PR is basically just a quick solution whist crypto v2 is still a labs feature. What is your specific question about throwing an error? |
I was just wondering why we don't throw the error if something bad happens during handleSync. But since that was already the case, I guess it's ok. |
|
Something weird happened with crypto tests (even locally), not related to this PR. I'm investigating it separately so going to merge this PR anyway |
The background crypto process (used to decrypt notifications) relies on several rust-crypto-sdk methods that are not multiprocess safe. In particular, since the background's
MXCryptoMachinemay live in memory for days, whereas foreground's will be recreated on each app launch, we may end up in a situation where foreground has written changes to disk that the background machine does not pick up. For more details see matrix-org/matrix-rust-sdk#1415This will be fixed in the future in the rust-sdk via a
proc_lockor similar multiprocess lock, but in a meanwhile a relatively quick solution is to recreateMXCryptoMachineon each operation (recieve sync, decrypt event), ensuring that each time we have relatively fresh data taken straight from disk.We could addionally add
NSFileCoordinatorto provide full mutual exclusion between the processes, but this adds some amount of complexity that will be eventually moved out to rust anyway. The chance of foreground and background processing sync at the same time is minimized by explicitsession.statechecks.