Page MenuHomePhabricator

Schedule blob deletion on iOS
ClosedPublic

Authored by marcin on Sep 6 2023, 4:10 AM.
Tags
None
Referenced Files
F1667987: D9091.id38780.diff
Fri, Apr 26, 2:31 PM
Unknown Object (File)
Tue, Apr 16, 10:14 PM
Unknown Object (File)
Tue, Apr 16, 6:24 PM
Unknown Object (File)
Tue, Apr 16, 2:57 PM
Unknown Object (File)
Fri, Apr 12, 3:41 AM
Unknown Object (File)
Mon, Apr 8, 5:11 AM
Unknown Object (File)
Mon, Apr 1, 12:56 PM
Unknown Object (File)
Sat, Mar 30, 7:50 AM

Details

Summary

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.

Test Plan
  1. Build iOS app connected to locally running blob service.
  2. Send a couple of large notifications.
  3. Kill the app.
  4. 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:

  1. Add line that logs blob holder when deletion is scheduled.
  2. Deploy the app to two different iOS devices and send large notification.
  3. Open the app on each device and using console app ensure that each of them logs different holder.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin requested review of this revision.Sep 6 2023, 4:31 AM
marcin added inline comments.
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:

tomek requested changes to this revision.Sep 7 2023, 2:22 AM

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:

  1. We read and delete
  2. Try to send a request
  3. We write again if the request failed

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:

  1. Read
  2. Send requests
  3. Delete

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?
If we're instantly resuming, can't we just run it as a plain code with try-catch?
Shouldn't we resume tasks only after all of them are created?

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.

This revision now requires changes to proceed.Sep 7 2023, 2:22 AM
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:

  1. Write one
  2. Read all
  3. Delete all

(In fact reading all instantly deletes all but it is straightforward to refactor it to support those operations separately).
If we could delete the specific request we could apply the second approach where we read all at once, do the networking and delete only the successful ones. Theoretically we could achieve it by writing each holder to a separate file, but my intuition is that is not the right direction to follow.

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:

  1. Application creates the request and submits it to the background session object.
  2. Request is executed in separate process at the time the OS finds most appropriate. Request execution is independent of the main app activity.
  3. When request completes successfully or fails the OS wakes up our app (or not if it was active) and calls relevant method of AppDelegate where we can handle success or failure.

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.
Basically this resume means restart or start if it has not been started previously. In this case it starts the task.

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).

  1. Delete blobs even if app is running in the background.
  2. Fix type in ongoing.

One last question: Is it guaranteed that we call the notif's completionHandler only after the deletion tasks are finished?

This revision is now accepted and ready to land.Sep 11 2023, 4:15 AM

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.

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.

Ok, that makes sense

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.

This revision is now accepted and ready to land.Mar 19 2024, 3:58 AM
tomek requested changes to this revision.Mar 22 2024, 9:42 AM

Similar to Android - shouldn't we have a separate holder for each recipient?

This revision now requires changes to proceed.Mar 22 2024, 9:42 AM

Send different holder to each recipient device

tomek added inline comments.
keyserver/src/push/send.js
1053 ↗(On Diff #38780)

Is there a way of avoiding changing const to let?

keyserver/src/push/utils.js
402 ↗(On Diff #38780)

Maybe numberOfHolders?

This revision is now accepted and ready to land.Mon, Apr 8, 6:42 AM
keyserver/src/push/send.js
1053 ↗(On Diff #38780)

I couldn't think of an alternative that wouldn't require ugly ternary operators.

Address Tomek suggestions

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?

Avoid using let instead of const.

This revision was automatically updated to reflect the committed changes.