This differential moves notifications crypto account to MMKV on native
Details
Repeat test plan for the parent diff. If another device is able to create outbound session it means that key upload callback are working
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
We should be really careful in reviewing this diff. It seems complex, and notif storage in C++ has historically been an area where we've often introduced very serious issues
- Don't pass references to unique pointer between lambdas - a rookie mistake!. Pass shared pointers by copy.
- Switch to multi process mode on MMKV on Android
native/android/app/src/main/java/app/comm/android/fbjni/CommMMKV.java | ||
---|---|---|
23 ↗ | (On Diff #42235) | It took me 8 hours of debugging to get there. This diff worked fine on iOS but on Android the app was crashing. I established that notifs crypto account was either missing in mmkv after setting it or it was set partially (subsequence of bytes)- it resulted in strange UTF8 encoding JNI crashes. Why does my MMKV instance lost data after App restart? There are some possibilities: You forget to set the multi-process mode to open a multi-process MMKV instance. Try setting multi-process mode to everywhere that access this MMKV instance. Check if it happens or not. I realized what was going on. MMKV keeps some data in memory and some on the disk. According to some heuristics it flushes data in memory to disk from time to time. In MULTI_PROCESS_MODE however it flushes everything immediately resulting in consistent data after each write. This explained iOS specificity - on iOS we used MULTI_PROCESS_MODE from the start since we shared it between NSE and the app. |
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
---|---|---|
580–584 ↗ | (On Diff #42235) | with that rollback is no longer working for notifs module - should we implement this too? How this could be handled in MMKV? |
656–657 ↗ | (On Diff #42235) | Not sure if this is safe - wondering if it's possible that if we don't remove notif account from SQLite it will cause some issues and override the already updated state in MMKV, but I don't see anything yet |
native/cpp/CommonCpp/NativeModules/CommCoreModule.h | ||
53–55 ↗ | (On Diff #42235) | overall I think that right now NotificationsCryptoModule should fully handle reading and persisting this - we have mixed responsibilities, and I see no reason for doing this here anymore. I feel like updating each NotificationsCryptoModule method to read content and persist makes the most sense and makes the code here cleaner. The only reason for that is being able to rollback persisting, but let's discuss this separately in my comment below. This is just an idea, wondering about your thoughts. Also given that this is already implemented and tested we can proceed and refactor this in the future. |
native/cpp/CommonCpp/NativeModules/CommCoreModule.h | ||
---|---|---|
53–55 ↗ | (On Diff #42235) | If you take a look at validateAndUploadPrekey you will see that we might want to execute a sequence operations governed by if conditions before persisting account state. With the design that you suggest we would have to persist after each call. This would result in unnecessary persists. We could refactor NotificationsCryptoModule to become statefull but this would look similar to what we already have in this differential. |
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
---|---|---|
580–584 ↗ | (On Diff #42235) | I intentionally put notifs persistence after content. If content persistence fails we won't even approach notifs persistence. On the other hand if we persist both accounts but there is a crash prior to commit call we won't be able to correctly rollback notifs account - MMKV doesn't provide transactional features. |
native/cpp/CommonCpp/NativeModules/CommCoreModule.h | ||
---|---|---|
53–55 ↗ | (On Diff #42235) | okay, makes sense |