Implements operation that reads all messages that were written since last call to this method, and clears main storage content if semaphore acquisition succeded. It also reads and deletes all random files with single notifications.
Details
Details
Use this method in AppDelegate to read messages written by NotificationService (see test plan for parent diff). Ensure all messages sent via web client are seen in the debugger where AppDelegate calls this method. Once it is done. Launch debugger again (but do not send any notifications). Ensure that no messages are seen this time (this checks for successful flush operation).
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
We should never see "No test plan". @marcinwasowicz, can you go through each diff you put up and add a test plan?
| native/ios/Comm/TemporalMessageStorage/TemporalMessageStorage.mm | ||
|---|---|---|
| 94 ↗ | (On Diff #14168) | Is it possible that while iterating the files a notification service extension would create a new file in the directory? How would we handle that? |
| 110 ↗ | (On Diff #14168) | We should do it in @finally |
| 111–114 ↗ | (On Diff #14168) | What's the purpose of this branch? If we read the file even if we haven't acquired the lock, why should we try to acquire it at all? |
| 138 ↗ | (On Diff #14168) | What's the purpose of it? |
| native/ios/Comm/TemporalMessageStorage/TemporalMessageStorage.mm | ||
|---|---|---|
| 94 ↗ | (On Diff #14168) | The file created in such a case will be read and deleted next time user launches app. |
| 110 ↗ | (On Diff #14168) | In my opinion there are no clear reasons to do so. @finally is a part of @try-catch statement that is not needed here since none of the functions used here throws exception. Additionally there are 3 cases here:
The only case we should try to release semaphore is 2. We could wrap else if() content in a try-catch statement with, but none of the methods called here throws exception so it might be redundant. |
| 111–114 ↗ | (On Diff #14168) | We try to acquire the lock to be able to clear file content without the worry that NotificationService will write something in the meantime. If we fail to acquire the lock, then it means NotificationService is writing something so we can still read its content but we cannot clear it. |
| 138 ↗ | (On Diff #14168) | componentsSeparatedByString:encryptedDataSeparator may return array containing empty strings if separator happens to be at the end, beginning or repeated in consecutive places. |
| native/ios/Comm/TemporalMessageStorage/TemporalMessageStorage.mm | ||
|---|---|---|
| 110 ↗ | (On Diff #14168) | After some considerations I came to a conclusion that reading a file might throw an exception if its content is too large. Therefore it it understandable to wrap else if content in a try catch statement. |
Comment Actions
I'm still not sure about the algorithm. Now we're reading the file without acquiring the lock, and got the lock only before deleting. What if we read file's content, then a service extension appends something to it, finishes, and we successfully acquire the lock and delete the file with unprocessed notif. We will lose all the notifs that were written after we read the file and before the lock.
| native/ios/Comm/TemporaryMessageStorage/TemporaryMessageStorage.mm | ||
|---|---|---|
| 128 ↗ | (On Diff #14278) | If !storageUpdated we don't call tryAcquireLock and we should probably avoid calling releaseLock, right? |
Comment Actions
You are right, I think reading should be placed inside a lock too, if it was acquired.
| native/ios/Comm/TemporaryMessageStorage/TemporaryMessageStorage.mm | ||
|---|---|---|
| 128 ↗ | (On Diff #14278) | releaseLock is a noop if we didn't acquire lock. In such a case it overwrites NSError** variable passed via arguments with information about an attempt to release lock that was not acquired in the first place. This message can be ignored. Another solution is to include an if statement in @finallly before we try to release lock, that checks if the lock was acquired. But this would increase indenation so I didn't choose this path. |
| native/ios/Comm/TemporaryMessageStorage/TemporaryMessageStorage.mm | ||
|---|---|---|
| 136 ↗ | (On Diff #14356) | Is it possible that we will see this file in the middle of notification service writing to it? |
| 128 ↗ | (On Diff #14278) | Ok, we can keep it for now, but in the future, if we decide to take different action when releasing the semaphore fails, we can revisit it. |
| native/ios/Comm/TemporaryMessageStorage/TemporaryMessageStorage.mm | ||
|---|---|---|
| 136 ↗ | (On Diff #14356) | Files handled in this else {} branch are the randomly named files NotificationService created on semaphore acquisition failure. They are written to with [EncryptedFileUtils WriteData: toFileAtPath: error:] method which uses [NSData writeToFile: atomically:YES]. Therefore if the write is atomic, AppDelegate cannot see this file in the middle ofNotificationService writing to it. |
| native/ios/Comm/TemporaryMessageStorage/TemporaryMessageStorage.mm | ||
|---|---|---|
| 136 ↗ | (On Diff #14356) | Makes sense, thanks! |