-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat(messaging, ios): Background Completion Handler from JS #8128
base: main
Are you sure you want to change the base?
Conversation
If you proposed a PR that stored the Promise result of the handler here:
...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 |
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. 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? |
I don't have any concrete thoughts sorry - I'm more of an Android person, thus my 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 |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
DescriptionAdded 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
Test PlanI have tested this on my app, and noticed the app spends less time in background, improving performance. 🔥 |
packages/messaging/ios/RNFBMessaging/RNFBMessaging+AppDelegate.m
Outdated
Show resolved
Hide resolved
my apologies, I missed that when porting in my implementation. I am not familiar with objective-c and not sure if this is flawless. 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. |
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.
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
@@ -219,6 +219,19 @@ - (NSDictionary *)constantsToExport { | |||
return resolve(@([RCTConvert BOOL:@(notifCenter.isHeadless)])); | |||
} | |||
|
|||
RCT_EXPORT_METHOD(completeNotificationProcessing) { | |||
RNFBMessagingAppDelegate *appDelegate = [RNFBMessagingAppDelegate sharedInstance]; | |||
if (appDelegate.completionHandler) { |
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.
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), ^{`
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