use dataSync service on receiving FCM notifications #3312
Conversation
r10s
changed the title
|
To test the changes in this pull request, install this apk: |
|
To test the changes in this pull request, install this apk: |
|
To test the changes in this pull request, install this apk: |
This reverts commit b7e5bee.
|
To test the changes in this pull request, install this apk: |
r10s
force-pushed
the
r10s/dataSync-servive-for-push
branch
2 times, most recently
from
01eaeee
to
6378423
Compare
this avoids potential issues with GenericForegroundService which eg. may block app start.
r10s
force-pushed
the
r10s/dataSync-servive-for-push
branch
from
6378423
to
dff11ad
Compare
r10s
changed the title
|
To test the changes in this pull request, install this apk: |
src/gplay/java/org/thoughtcrime/securesms/notifications/FcmReceiveService.java
Outdated
Show resolved
Hide resolved
| // to support accounts that do not send PUSH notifications and for simplicity, | ||
| // we just let the app run as long as possible. | ||
| FetchForegroundService.start(this); | ||
| if (!ApplicationContext.dcAccounts.backgroundFetch(19)) { // we should complete within 20 seconds |
There was a problem hiding this comment.
Can this call be moved inside the FetchForegroundService?
Otherwise it seems we create a FetchForegroundService that does nothing except showing a notification, but then immediately start fetching messages.
If starting FetchForegroundService takes some time or somehow fails then we fetch the messages without the service actually starting and may not have network access yet.
There was a problem hiding this comment.
i abandoned the idea initially, as it requires an additional worker thread in FetchForegroundService.onCreate() - whereas FcmReceiveService.onMessageReceived() already arrives from a worker thread. and for "just running" code, that seems fine.
If starting FetchForegroundService takes some time or somehow fails then we fetch the messages without the service actually starting and may not have network access yet.
not having internet or being killed too soon indeed is a strong argument, so thanks for the pointer, i added a commit.
it is indeed cleaner and more logical that way - and also when it comes to other push handlers, it is good to have things more concentrated in once class
|
To test the changes in this pull request, install this apk: |
| // We then run not longer than the max. of 20 seconds, | ||
| // see https://firebase.google.com/docs/cloud-messaging/android/receive . |
There was a problem hiding this comment.
I understood https://firebase.google.com/docs/cloud-messaging/android/receive differently:
- FirebaseMessagingService "should handle any message within 20 seconds of receipt", i.e. onMessageReceived should return within 20s
- "The time window for handling a message may be shorter than 20 seconds"
- Therefore, in the code example, they try to need only 10 seconds to be on the safe side:
if (/* Check if data needs to be processed by long running job */ true) {
// For long-running tasks (10 seconds or more) use WorkManager.
scheduleJob();
} else {
// Handle message within 10 seconds
handleNow();
}
- As soon as we are in a foreground service, we may need as long as we need to (within some bounds, IIRC on newer Androids you told me it's 6 hours per day or until the user swipes away the notification, anyway, it's way more than 20s)
Conclusion: IIUC, we can set the timeout to more than 20s, and I personally think that we should because on a slow network or so it may be nice to have a minute or even more.
There was a problem hiding this comment.
As soon as we are in a foreground service, we may need as long as we need to
indeed, that makes sense to me as well. once there is a foreground service notification, one is usually allowed to run long times - things are visible to the user. only without a foreground service notification one has 20 seconds - and maybe we got hit by that. though, still a bit confusing in the documentation ... thanks for the pointer!
what would be a reasonable timeout for slow networks then? 60 seconds? the service is not started a second time anyways in case another push arrives and we're not completed.
There was a problem hiding this comment.
Yes, I also wrote in the issue that datasync foreground task is allowed to run for 6 hours or so.
what would be a reasonable timeout for slow networks then? 60 seconds?
60 seconds is the timeout we use everywhere in the core (for connection timeout, read and write timeouts), but for the whole download I'd use 300 seconds (5 minutes) or something like this.
There was a problem hiding this comment.
Yes, I also wrote in the issue that datasync foreground task is allowed to run for 6 hours or so.
IIRC it is 6 hours for the whole day, so if you use it for several minutes in every push notification you might run out of "quota"
There was a problem hiding this comment.
i changed the timeout to 300 seconds in the last commit and adapted the comment
There was a problem hiding this comment.
only without a foreground service notification one has 20 seconds - and maybe we got hit by that.
I assume that what we got hit by is that we immediately returned in onMessageReceived(), so that the Android system assumed that we're done fetching. So, probably, just putting a Thread.sleep(10000) into onMessageReceived() would probably also have helped, but it would have drained more battery than necessary.
The "ideal" solution that only requires a foreground service (with "Updating..." notification) when necessary, and doesn't introduce additional network round trips, would probably be:
- Create a core function
dc_accounts_wait_for_new_messages(timeout)that callsmaybe_network()and then waits until all messages were fetched and returns true. If the timeout is over, it returns false, but doesn't stop the message-receiving;DC_EVENT_ACCOUNTS_BACKGROUND_FETCH_DONEwill then later be emitted when all messages were received. - In
onMessageReceived():
if (!dc_accounts_wait_for_new_messages(10)) {
FetchForegroundService.start(this);
}- Hope that there are no race conditions where
DC_EVENT_ACCOUNTS_BACKGROUND_FETCH_DONEarrives right beforeFetchForegroundServiceis started
There was a problem hiding this comment.
The "ideal" solution that only requires a foreground service (with "Updating..." notification) when necessary
yeah, that may or may not work, probably hard to say without quite some testing across different android versions, manufactors etc.
might be, systems require foreground service and grant network access only there. at least, this was the reasoning to move backgroundFetch() inside FetchForegroundService above.
so, i think, the current approach is kind of more bullet proof as it has less state and is pretty straight forward. let's see if the additional notification is really annoying and visible in case there is good connectivity
src/main/java/org/thoughtcrime/securesms/service/FetchForegroundService.java
Outdated
Show resolved
Hide resolved
this PR marks the code running after receiving an FCM push notification as a "foreground service", aiming not being killed. the "foreground service" requires a visible notification (nb: while we call this "background fetch", from the view of android, this is a foreground service, see https://developer.android.com/develop/background-work/services , most typical "background" things are "foreground services" nowadays)
when we're done fetching, "foreground service" is stopped, this also removes the notification.
in most cases, this is only a second or less and the notification should not really be visible
we are using these kind of "foreground service" already for a bunch of things - backup, key export, setup, therefore, i was first trying to use existing
GenericForegroundService. however,GenericForegroundServicehas the additional side effect to change the flow of activities (one does not want to eg. open the chatlist, when eg. a setup is not yet done), i did not want to add extra states to this already critical area.(also
GenericForegroundServicemight be more complicated as it needs to be - the newFetchForegroundServicelooks so much more straight forward :)closes #3281