Page MenuHomePhabricator
Differential D6776

Implement method in NotificationsCryptoModule to serialize CryptoModule instance and atomically store it in a file.
ClosedPublic
Actions

Authored by marcin on Feb 20 2023, 4:39 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 28, 1:53 AM
Unknown Object (File)
Thu, Nov 28, 12:47 AM
Unknown Object (File)
Thu, Nov 28, 12:34 AM
Unknown Object (File)
Wed, Nov 13, 4:01 AM
Unknown Object (File)
Fri, Nov 8, 7:29 PM
Unknown Object (File)
Fri, Nov 8, 3:49 PM
Unknown Object (File)
Oct 31 2024, 4:05 PM
Unknown Object (File)
Oct 26 2024, 10:54 PM
View All Files
Subscribers
ashoat
atul

Details

Reviewers
tomek
bartek
kamil
Commits
rCOMMc9a55c72dd19: Implement method in NotificationsCryptoModule to serialize CryptoModule…
Summary

This differential implements CryptoModule serialization and atomic write to a file under given path. Its implementation ensures that write is made atomically and in fault-tolerant manner.

Test Plan

Temporarily make this and deserialization method public. In CommCoreModule in initializeCryptoAccount method pass created CryptoModule instance, path returned from PlatformSpecificTools::getNotifications... and some random string (as pickling key) to this method. Then
pass the same pickling key and path returned from PlatformSpecificTools::getNotifications... to NotificationsCryptoModule::deserializeCryptoModule. Using XCode debugger or logging ensure initial CryptoModule matches the one returned from this method.
Note that it is an e2e test that also tests previous differential in more reliable manner.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin created this revision.Feb 20 2023, 4:39 AM
Herald added subscribers: atul, ashoat. · View Herald TranscriptFeb 20 2023, 4:39 AM
marcin added a parent revision: D6775: Implement method to deserialize olm account from a file.Feb 20 2023, 4:40 AM
marcin edited the test plan for this revision. (Show Details)
marcin published this revision for review.Feb 20 2023, 4:50 AM
marcin added inline comments.
native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
69 ↗(On Diff #22712)

We use this argument to create a temporary file path. We could create temporary file path using random string but what if any of the steps between temporary file creation and temporary file rename fails? We would be left with some garbage that should be deleted. We could use directory tree search POSIX api to get the path of such garbage temporary file but it would add additional complexity to the solution.
I expect that when calling this from main app we will provide "Comm" as calling process name, and "NotificationService" when calling from NotificationService.

marcin added a child revision: D6777: Implement public method to initialize (generate and persist new) notifications olm account.Feb 20 2023, 4:56 AM
Harbormaster failed remote builds in B16606: Diff 22712!Feb 20 2023, 7:35 AM
marcin updated this revision to Diff 22880.Feb 21 2023, 10:49 AM

Rebase

Harbormaster completed remote builds in B16725: Diff 22880.Feb 21 2023, 11:06 AM
bartek added inline comments.Feb 22 2023, 12:41 AM
native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
119

If file at path already exists, this is expected to overwrite it, right?

Reading the docs says this behavior is stdlib implementation-dependant:

If newname names an existing file, the function may either fail or override the existing file, depending on the specific system and library implementation.

marcin added inline comments.Feb 22 2023, 2:37 AM
native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
119

Can you link the docs you found? The ones I was using are explicit that existing file will be replaced:

  1. https://man7.org/linux/man-pages/man2/rename.2.html
  2. https://pubs.opengroup.org/onlinepubs/000095399/functions/rename.html
bartek added inline comments.Feb 23 2023, 12:34 AM
native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
119

I optimistically suppose that most implementations replace the file anyway. If we want to be 100% sure, we could simply test it on iOS and Android.

marcin added inline comments.Feb 24 2023, 2:55 AM
native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
119

Running the test plan on a freshly installed app should test it since on a freshly installed app the file (under the path returned by PlatformSpecificTools::getNotificationsCryptoAccountPath) does not exists so rename method is called with non-existing file path as newname argument. I executed this test plan on a fresh install and it worked as expected.

bartek accepted this revision.Feb 24 2023, 5:01 AM
bartek added inline comments.
native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
119

Ok, thanks for explaining

This revision is now accepted and ready to land.Feb 24 2023, 5:01 AM
marcin updated this revision to Diff 23254.Feb 28 2023, 10:06 AM

Rebase before landing

Harbormaster failed remote builds in B17000: Diff 23254!Feb 28 2023, 10:18 AM
Harbormaster completed remote builds in B17000: Diff 23254.Feb 28 2023, 1:43 PM
marcin updated this revision to Diff 23308.Mar 1 2023, 1:21 AM

Rebase before landing

Harbormaster completed remote builds in B17025: Diff 23308.Mar 1 2023, 1:36 AM
Closed by commit rCOMMc9a55c72dd19: Implement method in NotificationsCryptoModule to serialize CryptoModule… (authored by marcin). · Explain WhyMar 1 2023, 2:04 AM
This revision was automatically updated to reflect the committed changes.
marcin added a commit: rCOMMc9a55c72dd19: Implement method in NotificationsCryptoModule to serialize CryptoModule….

Revision Contents
Changeset List

PathSize
native/
cpp/
CommonCpp/
Notifications/
BackgroundDataStorage/
64 lines

Diff 22880

View Options

native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp

Loading...