Skip to content
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.

Sign up for GitHub

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

Jump to bottom

use dataSync service on receiving FCM notifications #3312

Merged
merged 9 commits into from
Sep 23, 2024
Merged

use dataSync service on receiving FCM notifications #3312

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
143713b
add backgroundFetch() api
r10s Sep 20, 2024
87d515f
call backgroundFetch() from FCM
r10s Sep 20, 2024
ab73a74
show a foreground service notification
r10s Sep 20, 2024
b7e5bee
add pinActivity parameter to GenericForegroundService
r10s Sep 21, 2024
f0da58b
Revert "add pinActivity parameter to GenericForegroundService"
r10s Sep 21, 2024
dff11ad
use explicit FetchForegroundService
r10s Sep 21, 2024
4dc5324
add reference for the 20 seconds time span
r10s Sep 22, 2024
dff2d98
move backgroundFetch() to FetchForegroundService
r10s Sep 22, 2024
b827643
as we called startForeground(), longer timeouts should be fine
r10s Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions jni/dc_wrapper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,12 @@ JNIEXPORT void Java_com_b44t_messenger_DcAccounts_setPushDeviceToken(JNIEnv *env
}


JNIEXPORT jboolean Java_com_b44t_messenger_DcAccounts_backgroundFetch(JNIEnv *env, jobject obj, jint timeout_seconds)
{
return dc_accounts_background_fetch(get_dc_accounts(env, obj), timeout_seconds) != 0;
}


JNIEXPORT jint Java_com_b44t_messenger_DcAccounts_addAccount(JNIEnv *env, jobject obj)
{
return dc_accounts_add_account(get_dc_accounts(env, obj));
Expand Down
7 changes: 3 additions & 4 deletions src/gplay/java/org/thoughtcrime/securesms/notifications/FcmReceiveService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

import org.thoughtcrime.securesms.ApplicationContext;
import org.thoughtcrime.securesms.BuildConfig;
import org.thoughtcrime.securesms.util.Prefs;
import org.thoughtcrime.securesms.service.FetchForegroundService;
import org.thoughtcrime.securesms.util.Util;

public class FcmReceiveService extends FirebaseMessagingService {
Expand Down Expand Up @@ -92,12 +92,11 @@ public static String getToken() {
return prefixedToken;
}

@WorkerThread
@Override
public void onMessageReceived(@NonNull RemoteMessage remoteMessage) {
Log.i(TAG, "FCM push notification received");
// the app is running (again) now and fetching and notifications should be processed as usual.
// 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);
}

@Override
Expand Down
4 changes: 4 additions & 0 deletions src/main/AndroidManifest.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,10 @@
android:name=".service.GenericForegroundService"
android:foregroundServiceType="dataSync" />

<service
android:name=".service.FetchForegroundService"
android:foregroundServiceType="dataSync" />

<service
android:name=".service.IPCAddAccountsService"
android:foregroundServiceType="dataSync"
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/b44t/messenger/DcAccounts.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ public void unref() {
public native void stopIo ();
public native void maybeNetwork ();
public native void setPushDeviceToken (String token);
public native boolean backgroundFetch (int timeoutSeconds);

public native int addAccount ();
public native int migrateAccount (String dbfile);
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/b44t/messenger/DcContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ public class DcContext {
public final static int DC_EVENT_WEBXDC_STATUS_UPDATE = 2120;
public final static int DC_EVENT_WEBXDC_INSTANCE_DELETED = 2121;
public final static int DC_EVENT_WEBXDC_REALTIME_DATA = 2150;
public final static int DC_EVENT_ACCOUNTS_BACKGROUND_FETCH_DONE = 2200;

public final static int DC_IMEX_EXPORT_SELF_KEYS = 1;
public final static int DC_IMEX_IMPORT_SELF_KEYS = 2;
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/org/thoughtcrime/securesms/connect/DcEventCenter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

import org.thoughtcrime.securesms.ApplicationContext;
import org.thoughtcrime.securesms.R;
import org.thoughtcrime.securesms.service.FetchForegroundService;
import org.thoughtcrime.securesms.util.Util;

import java.util.ArrayList;
Expand Down Expand Up @@ -177,6 +178,10 @@ public long handleEvent(@NonNull DcEvent event) {
DcHelper.getNotificationCenter(context).removeNotifications(accountId, event.getData1Int());
break;

case DcContext.DC_EVENT_ACCOUNTS_BACKGROUND_FETCH_DONE:
FetchForegroundService.stop(context);
r10s marked this conversation as resolved.
Show resolved Hide resolved
break;

case DcContext.DC_EVENT_IMEX_PROGRESS:
sendToCurrentAccountObservers(event);
return 0;
Expand Down
1 change: 1 addition & 0 deletions src/main/java/org/thoughtcrime/securesms/notifications/NotificationCenter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ private PendingIntent getMarkAsReadIntent(ChatData chatData, int msgId, boolean
public static final int ID_PERMANENT = 1;
public static final int ID_MSG_SUMMARY = 2;
public static final int ID_GENERIC = 3;
public static final int ID_FETCH = 4;
public static final int ID_MSG_OFFSET = 0; // msgId is added - as msgId start at 10, there are no conflicts with lower numbers


Expand Down
79 changes: 79 additions & 0 deletions src/main/java/org/thoughtcrime/securesms/service/FetchForegroundService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
package org.thoughtcrime.securesms.service;

import android.app.Notification;
import android.app.Service;
import android.content.Context;
import android.content.Intent;
import android.os.IBinder;
import android.util.Log;

import androidx.annotation.Nullable;
import androidx.core.app.NotificationCompat;
import androidx.core.content.ContextCompat;

import org.thoughtcrime.securesms.ApplicationContext;
import org.thoughtcrime.securesms.R;
import org.thoughtcrime.securesms.notifications.FcmReceiveService;
import org.thoughtcrime.securesms.notifications.NotificationCenter;
import org.thoughtcrime.securesms.util.Util;

public final class FetchForegroundService extends Service {
private static final String TAG = FcmReceiveService.class.getSimpleName();
private static final Object SERVICE_LOCK = new Object();
private static Intent service;

public static void start(Context context) {
GenericForegroundService.createFgNotificationChannel(context);
synchronized (SERVICE_LOCK) {
if (service == null) {
service = new Intent(context, FetchForegroundService.class);
ContextCompat.startForegroundService(context, service);
}
}
}

public static void stop(Context context) {
synchronized (SERVICE_LOCK) {
if (service != null) {
context.stopService(service);
service = null;
}
}
}

@Override
public void onCreate() {
Log.i(TAG, "Creating fetch service");
super.onCreate();

Notification notification = new NotificationCompat.Builder(this, NotificationCenter.CH_GENERIC)
.setContentTitle(getString(R.string.connectivity_updating))
.setSmallIcon(R.drawable.notification_permanent)
.build();

startForeground(NotificationCenter.ID_FETCH, notification);

// Start explicit fetch only after we marked ourselves as requiring foreground;
// this may help we on getting network and time adequately
// Fetch is started in background to not block the UI.
// We then run not longer than the max. of 20 seconds,
// see https://firebase.google.com/docs/cloud-messaging/android/receive .
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@r10s r10s Sep 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i changed the timeout to 300 seconds in the last commit and adapted the comment

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 calls maybe_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_DONE will 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_DONE arrives right before FetchForegroundService is started

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Util.runOnAnyBackgroundThread(() -> {
Log.i(TAG, "Starting fetch");
if (!ApplicationContext.dcAccounts.backgroundFetch(19)) {
FetchForegroundService.stop(this);
}
Hocuri marked this conversation as resolved.
Show resolved Hide resolved
});
}

@Override
public void onDestroy() {
stopForeground(true);
}

@Nullable
@Override
public IBinder onBind(Intent intent) {
return null;
}
}
2 changes: 1 addition & 1 deletion src/main/java/org/thoughtcrime/securesms/service/GenericForegroundService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ synchronized void replaceProgress(int id, int progressMax, int progress, boolean
}

@TargetApi(Build.VERSION_CODES.O)
static private void createFgNotificationChannel(Context context) {
static public void createFgNotificationChannel(Context context) {
if(!CHANNEL_CREATED.get() && Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
CHANNEL_CREATED.set(true);
NotificationChannel channel = new NotificationChannel(NotificationCenter.CH_GENERIC,
Expand Down
Loading