This differential implements blob deletion on iOS. NSE runs in separate process that has constrained execution time so low-priority deferable networking should not be executed there. Therefore NSE persists blob metadata in a
TemporaryMessageStorage. Then the main app reads the storage on its start and issues delete requests. All happens on a background thread so no UI operation is blocked.
Details
- Reviewers
tomek bartek atul - Commits
- rCOMM2ae576369e6b: Schedule blob deletion on iOS
- Build iOS app connected to locally running blob service.
- Send a couple of large notifications.
- Kill the app.
- Open it and ensure that for each notifications there is a corresponding DELETE request. It can be done either using local instance of blob service or by adding a log statement to successHandler argument to deleteBlobAsyncWithHash.
Additionally:
- Add line that logs blob holder when deletion is scheduled.
- Deploy the app to two different iOS devices and send large notification.
- Open the app on each device and using console app ensure that each of them logs different holder.
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-4247
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
native/ios/Comm/AppDelegate.mm | ||
---|---|---|
432 ↗ | (On Diff #30796) | This method of app delegate is called when app starts to transition into inactive state: https://developer.apple.com/documentation/uikit/uiapplicationdelegate/1622950-applicationwillresignactive?language=objc. The docs state that it is the place to stop various on going tasks. Why do we need to do this? Those HTTP calls are executing on the background thread but we can't just let them run when application enters the background state (user swipes app) since iOS might kill our app for this. To execute code when application is in the background iOS provides specific APIs:
|
Overall, it makes sense, but I have a couple of questions.
Shouldn't we also send a delete request when a notif is received while the app is running?
native/ios/Comm/AppDelegate.mm | ||
---|---|---|
412–428 ↗ | (On Diff #30796) | This approach is fragile:
If, at any point, something goes wrong (e.g. an exception is thrown), we will not write again the content. An alternative approach might be to:
By doing that, in the worst case, we would send a delete request twice. One issue is that after reading the messages, another process could write some new messages - we should think about this case. |
native/ios/Comm/CommIOSNotifications/CommIOSNotificationsBlobClient.h | ||
11 ↗ | (On Diff #30796) | Ongoing is a single word |
native/ios/Comm/CommIOSNotifications/CommIOSNotificationsBlobClient.mm | ||
114–128 ↗ | (On Diff #30796) | Why do we instantly resume a task? Disclaimer: I'm not sure that something is wrong with this approach, but I also don't know if the approach is correct - that's why I'm asking these questions. |
native/ios/Comm/AppDelegate.mm | ||
---|---|---|
412–428 ↗ | (On Diff #30796) | Both approaches has their pros and cons but I both cases they arise from the fact that our implementation of TemporaryMessageStorage supports only the following operations:
(In fact reading all instantly deletes all but it is straightforward to refactor it to support those operations separately). I examined this API: https://developer.apple.com/documentation/foundation/url_loading_system/downloading_files_in_the_background?language=objc since at first sight it looked promising. However after studying it I came to the conclusion that it basically works like this:
So basically the only benefit here is that the request is less likely to fail (since it is executed on a dedicated process) and we don't have to handle the app being put to background (which is not actually an issue as we do it with the applicationWillResignActive or we could do it with https://developer.apple.com/documentation/uikit/uiapplication/1623051-beginbackgroundtask). The second approach (read, send, delete) can still fail if NSE writes while we are executing requests. The first approach (read and delete, send, rewrite if send fails) can still fail due to exception being thrown since handling failure would take place in the main app code. So the conclusion is that the problem is not that iOS offers limited API to do networking in the background since the API is apparently quite rich. The problem is that we don't have a storage layer that would support deletion of specific holder rather than all at once. With such a storage we could easily make the code in this differential fault tolerant. The similar diff on Android (D9069) uses WorkManager API which uses its internal SQLite to persist failed requests so that they can be retried later. |
native/ios/Comm/CommIOSNotifications/CommIOSNotificationsBlobClient.mm | ||
114–128 ↗ | (On Diff #30796) | The name of this method is probably confusing without the docs: https://developer.apple.com/documentation/foundation/nsurlsessiontask/1411121-resume. |
Shouldn't we also send a delete request when a notif is received while the app is running?
This is possible using the darwin notification mechanism. We already use it so that NSE can inform the backgrounded running app that it can fetch messages from the temporary storage. One thing that I need to check however is that whether darwin notifications are delivered instantly even if the app is in the background or they are queued until the app enters the foreground. In our case we would prefer to second since I don't want to execute networking code when the app is backgrounded (the reason for this was explained in one of my first comments on this diff: https://phab.comm.dev/D9091#inline-57952).
One last question: Is it guaranteed that we call the notif's completionHandler only after the deletion tasks are finished?
One last question: Is it guaranteed that we call the notif's completionHandler only after the deletion tasks are finished?
It is not guaranteed and it is impossible to guarentee that since contentHandler is called quickly in the NSE while blob deletion takes place on the background thread in the main app so the system decides when it is best to call it. Additionally main app might not be running when NSE processes notification. However why would such guarantee be necessary in the first place? Blob download and persistence is synchronous and writing blob metadata to the storage which main app reads during deletion takes place afterwards. So the deletion process won't start until notification payload is persisted on the device so when user opens the app the message will be loaded and immediately visible in the app regardless on when delete requests are issued.
Using MMKV we can implement resilient blob deletion approach like we have on Android.
Renew the code. Reimplement blob deletion on top of MMKV in a resilent way - this time it is guarenteed that we will eventually delete blobs.
keyserver/src/push/send.js | ||
---|---|---|
1053 ↗ | (On Diff #38780) | I couldn't think of an alternative that wouldn't require ugly ternary operators. |
keyserver/src/push/send.js | ||
---|---|---|
1053 ↗ | (On Diff #38780) | One possible option would be to create a new variable in line 1105 - will that work? |