feat(messaging, ios): Background Completion Handler from JS

[Beat-YT]

What feature would you like to see?

When receiving a background message, the package will calls the completion handler after fixed 25 seconds, but this has some repercussion, considering that the system limit before the app gets killed is 30 seconds.

If an app stays in the background longer than necessary, iOS may reduce how often it triggers the app for future background tasks to save on battery, affecting performance and notification reliability over time.

An option to call the completion handler directly from JavaScript would be ideal. This would allow apps to signal when they are done with background processing, instead of relying on the default timeout, enhancing responsiveness and efficiency for iOS background tasks.

By taking a look at the source code, we can see that it was planned to add this feature, but it has yet to be supported.

react-native-firebase/packages/messaging/ios/RNFBMessaging/RNFBMessaging+AppDelegate.m

Lines 168 to 173 in 227e1ed

	// TODO add support in a later version for calling completion handler directly from JS when
	// user JS code complete
	dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(25 * NSEC_PER_SEC)),
	dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_HIGH, 0), ^{
	completionHandler(UIBackgroundFetchResultNewData);

[mikehardy]

If you proposed a PR that stored the Promise result of the handler here:

react-native-firebase/packages/messaging/lib/index.js

Line 101 in 227e1ed

return backgroundMessageHandler(remoteMessage);

...and called a new native API that ended the background task (and cancelled the 25-seconds-later dispatch?) then returned the Promise, that would probably be good to merge

[Beat-YT]

But do we even need to cancel the 25-second dispatch if we implement the direct completion handler call? Won't the whole app shutdown when the completion handler is called? My understanding is that once the completion handler is called, the background task should end, and the app should return to its suspended state, making the dispatch unnecessary.
Looking forward to your thoughts on these points.

Additionally, in my understanding, if we implement the completion handler on the JavaScript side and call it when the promise is resolved, wouldn't the 25-second dispatch timer become unnecessary?

[mikehardy]

I don't have any concrete thoughts sorry - I'm more of an Android person, thus my ? in that statement about cancelling the dispatch. I would only know after experimentation, personally.

I believe there is always the possibility someone could write a handler that is longer than the current timeout and maybe never completes. I'm not sure what the implication of never completing are, but if there is any consequence to that then there should be a fallback "complete at native level whether javascript signaled us or not", which implies the 25-second shutdown delayed dispatch may still be necessary

But again - I would have to experiment personally to know all the details and it isn't a priority for me personally - I was just trying to suggest what a PR might look like that could be merged, if it was important enough in your use case to implement

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-native-firebase	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 22, 2024 3:07pm

[Beat-YT]

Description

Added code to call the completion handler directly from JavaScript. This allow apps to signal when they are done with background processing, instead of relying on the default timeout, enhancing responsiveness and efficiency for iOS background tasks.

Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • Yes
• My change supports the following platforms;
  • Android
  • iOS
• My change includes tests;
  • e2e tests added or updated in packages/\*\*/e2e
  • jest tests added or updated in packages/\*\*/__tests__
• I have updated TypeScript types that are affected by my change.
• This is a breaking change;
  • Yes
  • No

Test Plan

I have tested this on my app, and noticed the app spends less time in background, improving performance.

🔥

[Beat-YT]

my apologies, I missed that when porting in my implementation.

I am not familiar with objective-c and not sure if this is flawless.
In my app's implementation I made the choice to call the completion handler from the main thread like so:

RCT_EXPORT_METHOD(completeNotificationProcessing) {
  dispatch_async(dispatch_get_main_queue(), ^{
    RNFBMessagingAppDelegate *appDelegate = [RNFBMessagingAppDelegate sharedInstance];
    if (appDelegate.completionHandler) {
      appDelegate.completionHandler(UIBackgroundFetchResultNewData);
      appDelegate.completionHandler = nil;
    }
    if (appDelegate.backgroundTaskId != UIBackgroundTaskInvalid) {
      [[UIApplication sharedApplication] endBackgroundTask:appDelegate.backgroundTaskId];
      appDelegate.backgroundTaskId = UIBackgroundTaskInvalid;
    }
  });
}

I am not sure if this is needed really, and I made the choice to not include it in this pull request.
Perhaps @russellwheatley knows better than me and can give his thoughts on this.

[mikehardy]

I think this looks really good - it appears this will work without regression or behavior change as it is, and should allow fast completion otherwise.

One comment about the exact thing you mentioned - the possibility of a race condition on the sharedInstance property state was the only thing that jumped out at me

If access to those properties was thread safe, I'd say +1 for sure

[mikehardy]

I think this should be on the same queue as the initial set of completionHandler / backgroundTaskId otherwise there's a tiny chance via race condition that multiple threads are working on their state at the same time

It's this above, would that work here?

                dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_HIGH, 0), ^{`

[github-actions]

Hello 👋, this PR has been opened for more than 2 months with no activity on it.

If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing!

You have 15 days until this gets closed automatically