feat: foreground service for sending pending messages (WPB-26422)#4971
feat: foreground service for sending pending messages (WPB-26422)#4971sbakhtiarov wants to merge 1 commit into
Conversation
Ups 🫰🟨This PR is too big. Please try to break it up into smaller PRs. |
3e5917a to
d2ec791
Compare
Codecov Report❌ Patch coverage is Please upload reports for the commit c78640a to get more accurate results. ❌ Your patch check has failed because the patch coverage (36.31%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #4971 +/- ##
===========================================
- Coverage 48.86% 48.68% -0.18%
===========================================
Files 646 646
Lines 23215 23319 +104
Branches 3563 3568 +5
===========================================
+ Hits 11343 11353 +10
- Misses 10813 10917 +104
+ Partials 1059 1049 -10
... and 14 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
| scope.launch { | ||
| run(userId) | ||
| stopSelf(startId) | ||
| } |
There was a problem hiding this comment.
I see a problem here - onStartCommand can be executed multiple times, so for instance when you have 2 accounts logged in and both have some pending messages - then ACTION_SENDING_OF_PENDING_MESSAGES_SCHEDULED will be called twice with each userId and here two coroutines will start for each user.
It's using stopSelf with startId, but this logic is designed for sequential or single-task services, it does not understand concurrent operations, like multiple userIds processing side-by-side. If a newer startId completes first, it tells the OS "I am the latest command, we can shut down now," which triggers onDestroy() and risks killing the entire process while the first userId is still being processed, because the scope is cancelled in onDestroy, or the OS may cut off the internet and/or resources for the first coroutine which hasn't finished yet. 🤔
Also, if another onStartCommand appears when some other just called stopSelf but the service hasn't been fully closed, then OS may decide to keep that service, but scope is already closed and dead.
| PendingMessagesForegroundSyncHandler(coreLogic, sendPendingMessagesAfterForegroundSync).sendPendingMessagesForCurrentSession(userId) | ||
| } | ||
|
|
||
| private fun networkAvailability(): Flow<Boolean> = callbackFlow { |
There was a problem hiding this comment.
Can't we reuse our NetworkStateObserver here instead of implementing and registering another network callback?
|
|
||
| private fun createNotification(waitingForConnection: Boolean): Notification = | ||
| NotificationCompat.Builder(this, PENDING_MESSAGES_SYNC_CHANNEL_ID) | ||
| .setContentText( |
There was a problem hiding this comment.
We had some issues on huawei devices crashing with "bad foreground notification" when it didn't have setContentTitle as well
d2ec791 to
c78640a
Compare
|
@sbakhtiarov looks like you are rolling back kalium to a previous commitish. This means that the PR's target branch (develop) is using a newer version of Kalium, and the changes in this PR will rollback Kalium to an older version.
Is this intentional? |
|
https://wearezeta.atlassian.net/browse/WPB-26422
https://wearezeta.atlassian.net/browse/WPB-26422
What's new in this PR?
Issues
Current solution for sending pending messages after restoring Internet connection does not work when application is not in opened by user.
Causes (Optional)
Caused by recent Android restrictions for background work:
Solutions
Use foreground service which is waiting for connection and retry sending pending messages.