-
Notifications
You must be signed in to change notification settings - Fork 120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cleanup the background task that pauses the SDK when resigning active #903
Conversation
Generated by 🚫 Danger Swift against 81b3eee |
352e0af
to
9105b0b
Compare
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## develop #903 +/- ##
===========================================
- Coverage 49.31% 49.18% -0.13%
===========================================
Files 329 329
Lines 19979 19996 +17
Branches 11037 11038 +1
===========================================
- Hits 9852 9835 -17
- Misses 9847 9883 +36
+ Partials 280 278 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
I am actually wondering, do we really need this background task at all? Don't we just need to suspend the sync when the AppCoordinator sees that the app is suspending, and nothing more? |
fa7eb2f
to
16f4974
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me but I would like some api changes that I commented on inline.
Separately from this, the usage of the background tasks in the roomProxy feels fishy to me. For example sendMessage
starts a background task and then stops it on exiting the method. In reality the async call to the SDK doesn't return when the message was actually sent but when it was enqueued. I think that basically means that the background task here is useless. What do you think?
…mmediately foregrounded again, we do not start the sync
905a44d
to
6092331
Compare
I actually tested it and the background task is actually needed, I added an artificial delay before the enqueueing of let's say 3 seconds before the sending of the message, without that bg task it will only get sent when I reopen the app, instead with it gets sent in bg. |
Huh, interesting, okay then 👍 |
Kudos, SonarCloud Quality Gate passed!
|
From the video you can see that only the third time I background the app (where I waited about 30 seconds) when I foregrounded it, I got back the loading indicator showing the the sync has restarted.
So we are extending the duration of the sync for 30 seconds after suspension, and then stopping it in the expiration handler
We restart the sync only if the app is coming from a suspended state and the sync was stopped.
Simulator.Screen.Recording.-.iPhone.14.-.2023-05-17.at.17.53.12.mp4