perf(messaging, ios): Improve time to delivery of background messages on iOS

[brianathere]

Convert didReceiveRemoteNotification to wait until the RN bridge initialized based on the completion of the call to setBackgroundMessageHandler by the client

Description

Related issues

Release Summary

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

Tested delivery using Firebase messaging, messages with data, in the foreground and background on an actual device. Confirmed foreground notification delivery is still immediate. Confirmed background notification delivery occurs when the application is running but hidden, as well as when the application has been force quit.

As this requires an actual device and no existing test automation existed for these scenarios all of the testing was manual.

---

Think react-native-firebase is great? Please consider supporting the project with any of the below:

• 👉 Star this repo on GitHub ⭐️
• 👉 Follow React Native Firebase and Invertase on Twitter

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

react-native-firebase – ./

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/wa7UfKcgFvzRFyUjna7TVBocz7GM
✅ Preview: https://react-native-firebase-git-fork-heredotvideo-br-c3becb-invertase.vercel.app

react-native-firebase-next – ./website_modular

🔍 Inspect: https://vercel.com/invertase/react-native-firebase-next/5cV8ue1vQEWj1wNGmi7BrBPi8Ey2
✅ Preview: Canceled

[Deployment for fc2935c canceled]

[CLAassistant]

All committers have signed the CLA.

[mikehardy]

Very interesting! I love this idea, where I will summarize the idea as "solve the iOS 'when is the RN bridge up' detection issue by using the 'setBackgroundMessageHandler' call as a positive signal to uncork messages'

Did I get that correct, as the idea?

If so, it seems like a nice fix and I'd like to collaborate with you to get this merged and released.

I see these things as necessary to get it in shape though

• nothing in the tests/ directory should be included in this PR, they are not relevant to the fix and clutter the PR - please remove all those items
• the e2e test you changed is not executed on ios at all, and this is an ios fix, that change should be reverted
• this is currently not backwards compatible, and as the setBackgroundMessageHandler API is marked as "android" it's quite possible that people are not calling it on iOS but still receiving messages. If I understand the code change proposed here, iOS will now hang indefinitely on message delivery if setBackgroundMessageHandler is not called. The previous time-delay/uncork logic needs to remain as a fallback I think, leaving your idea of the positive signal given by setBackgroundMessageHandler as a very nice optimization but without potentially causing problems for existing users.

What do you think?

[brianathere]

Very interesting! I love this idea, where I will summarize the idea as "solve the iOS 'when is the RN bridge up' detection issue by using the 'setBackgroundMessageHandler' call as a positive signal to uncork messages'

Did I get that correct, as the idea?

If so, it seems like a nice fix and I'd like to collaborate with you to get this merged and released.

I see these things as necessary to get it in shape though

• nothing in the tests/ directory should be included in this PR, they are not relevant to the fix and clutter the PR - please remove all those items
• the e2e test you changed is not executed on ios at all, and this is an ios fix, that change should be reverted
• this is currently not backwards compatible, and as the setBackgroundMessageHandler API is marked as "android" it's quite possible that people are not calling it on iOS but still receiving messages. If I understand the code change proposed here, iOS will now hang indefinitely on message delivery if setBackgroundMessageHandler is not called. The previous time-delay/uncork logic needs to remain as a fallback I think, leaving your idea of the positive signal given by setBackgroundMessageHandler as a very nice optimization but without potentially causing problems for existing users.

What do you think?

Excellent feedback. I accidentally sent this up for review, well before I got started, so let me start again here in an hour with a working solution, and then let's go from there.

I wasn't aware setBackgroundMessageHandler was considered optional on iOS, and further saw it marked android which didn't make complete sense since it is needed on iOS, so yeah, I'll need to give how to mark this as backward breaking or not a think based on feedback.

[mikehardy]

Hmm - I was keying that comment mostly off the fact the API call was marked android and not taken into account before. Perhaps I am fearing for backwards compatibility too much and/or incorrectly.

I wasn't aware setBackgroundMessageHandler was considered optional on iOS

On a re-think, perhaps for this to matter at all, you would need a handler set? Stated differently: the only messages we are delaying here would be sent through the background message handler?

If so, then my assertion it is optional and needs management here is incorrect. I regret I'm not familiar enough with this exact area of code to know off-hand but it should be easy to verify the assertion, and I may be wrong...

[codecov]

Codecov Report

Merging #5547 (dba69ba) into master (832057c) will decrease coverage by 0.07%.
The diff coverage is 0.00%.

❗ Current head dba69ba differs from pull request most recent head fc2935c. Consider uploading reports for the commit fc2935c to get more accurate results

@@             Coverage Diff              @@
##             master    #5547      +/-   ##
============================================
- Coverage     67.56%   67.50%   -0.06%     
+ Complexity      982      979       -3     
============================================
  Files           199      199              
  Lines          9639     9641       +2     
  Branches       1504     1504              
============================================
- Hits           6512     6507       -5     
- Misses         2715     2719       +4     
- Partials        412      415       +3     

[brianathere]

Hmm - I was keying that comment mostly off the fact the API call was marked android and not taken into account before. Perhaps I am fearing for backwards compatibility too much and/or incorrectly.

I wasn't aware setBackgroundMessageHandler was considered optional on iOS

On a re-think, perhaps for this to matter at all, you would need a handler set? Stated differently: the only messages we are delaying here would be sent through the background message handler?

If so, then my assertion it is optional and needs management here is incorrect. I regret I'm not familiar enough with this exact area of code to know off-hand but it should be easy to verify the assertion, and I may be wrong...

Hmm - I was keying that comment mostly off the fact the API call was marked android and not taken into account before. Perhaps I am fearing for backwards compatibility too much and/or incorrectly.

I wasn't aware setBackgroundMessageHandler was considered optional on iOS

On a re-think, perhaps for this to matter at all, you would need a handler set? Stated differently: the only messages we are delaying here would be sent through the background message handler?

If so, then my assertion it is optional and needs management here is incorrect. I regret I'm not familiar enough with this exact area of code to know off-hand but it should be easy to verify the assertion, and I may be wrong...

My latest updates are what I was intending to share.

I believe nothing has changed in the case of foreground delivery with my change, it just gets sent to the app.

In the background delivery will be held on a dispatch queue until the client code calls setBackgroundMessageHandler and provides a callback. If the client code never calls, the delivery request will wait indefinitely.

Since setBackgroundMessageHandler is used on iOS and Android I removed the platform flag from that method.

I looked into writing automation for testing this change, however since it requires a real device to test I tested in my application instead. I'v only tested on an iPhone 12 at this point, but will go back to an iPhone 7 before merging.

As I've not written ObjC in 25 years, I'd very much appreciate eyes on that code.

[mikehardy]

I have some comments on logging and documentation but I don't have the Obj-C skills to know if I'm missing something. This is something that will affect the Notifee library a lot if there is an issue though, perhaps I could get a look from @helenaford ? For context if you have time Helena - the underlying idea seems sound to me, and the implementation in general even, to me - at this point I think we're just trying to suss out if if the code will actually spin-then-uncork messages correctly, with a side question (on my part) on what exactly will happen if no background handler is ever registered? (e.g., will it use 100% cpu, or ?)

[mikehardy]

This looks good to me know - as mentioned I myself am hoping for another set of eyes on the objective-c but I don't have any hangups personally and if no one else can take a look I'm biased towards merge.

Please note that this generates a patch set from the patch CI action (https://github.com/invertase/react-native-firebase/actions/runs/1069327151) - it generates the patch set as the full difference between current release (12.3.0 as of this typing) and current dev branch + this change so it contains more than just this PR, but stated differently it contains "everything approved for release plus the PR" so it's still worth trying to make sure it works for you in production via patch-package

In the absence of qualified objective-c review that's how I'll test integrate it in my app for verification as well, would love a report if you try it

[brianathere]

I have some comments on logging and documentation but I don't have the Obj-C skills to know if I'm missing something. This is something that will affect the Notifee library a lot if there is an issue though, perhaps I could get a look from @helenaford ? For context if you have time Helena - the underlying idea seems sound to me, and the implementation in general even, to me - at this point I think we're just trying to suss out if if the code will actually spin-then-uncork messages correctly, with a side question (on my part) on what exactly will happen if no background handler is ever registered? (e.g., will it use 100% cpu, or ?)

When didReceiveRemoteNotification is called and the app is in the background and the javascript client has not yet set a background message handler, a task is queued on the GCD thread pool, and the task waits for the client to set a handler. The task shouldn't use CPU, but it will consume a thread from the thread pool. This could lead to resource (address space, allowed threads?) exhaustion, if a large number (millions? - I don't know enough about modern iOS to know what the limits will be) of didReceiveRemoteNotification events are delivered. To mitigate this I added a change to the wait to give up after 25 seconds, aligned with the timeout for the beginBackgroundTaskWithExpirationHandler block. I tested a bit, seems to work well.

[brianathere]

This looks good to me know - as mentioned I myself am hoping for another set of eyes on the objective-c but I don't have any hangups personally and if no one else can take a look I'm biased towards merge.

Please note that this generates a patch set from the patch CI action (https://github.com/invertase/react-native-firebase/actions/runs/1069327151) - it generates the patch set as the full difference between current release (12.3.0 as of this typing) and current dev branch + this change so it contains more than just this PR, but stated differently it contains "everything approved for release plus the PR" so it's still worth trying to make sure it works for you in production via patch-package

In the absence of qualified objective-c review that's how I'll test integrate it in my app for verification as well, would love a report if you try it

Happy to try this, but I don't understand how to apply this patch. Can you point me to the documentation?

[mikehardy]

@brianathere oh sure! The main to know is that https://github.com/ds300/patch-package exists (and it's pretty easy to install), then having learned about patch-package, the patches.zip we generate makes sense - it has a few patches you drop in your ./patches/ directory and whenever you yarn (or npm i or whatever) they apply and that's that. A great way to carry local changes without forking a repo, or in our case here since we are a monorepo about the only way for people to easily integrate PRs here into their actual project

[mikehardy]

Unrelated: I just noticed the CLA wasn't signed yet either. Once things are reviewed I merge and release pretty quickly, so follow the details link on that pending check here and it'll help step through it

[brianathere]

Unrelated: I just noticed the CLA wasn't signed yet either. Once things are reviewed I merge and release pretty quickly, so follow the details link on that pending check here and it'll help step through it

Figured out the CLA, e-mail mismatch. Fixed.

[brianathere]

@brianathere oh sure! The main to know is that https://github.com/ds300/patch-package exists (and it's pretty easy to install), then having learned about patch-package, the patches.zip we generate makes sense - it has a few patches you drop in your ./patches/ directory and whenever you yarn (or npm i or whatever) they apply and that's that. A great way to carry local changes without forking a repo, or in our case here since we are a monorepo about the only way for people to easily integrate PRs here into their actual project

I've tested on an iPhone 7 on iOS 12.1, and on a 12 Max running 14.7, works well in my application in both locations. I see there is still a block on the CLA being signed, do I need to do something more to get this cleared and this change merged?

[mikehardy]

Sorry - just sweeping through notifications now (I do one big react-native-firebase pass a day, this is it). I do not believe there are any further actions required on your part.
I've asked for another set of Objective-C eyes on this just in case but I'm biased towards merge+release and will do so without review if I don't get a second look by this afternoon my time (that's a few hours from now).

[mikehardy]

Okay, just re-re-read this after the latest changes and it still LGTM. On a countdown for merge on my side, though it is possible we get a second review and they notice something I haven't

👍

[mikehardy]

I suppose the doc you just made better for #5545 is now no longer up to date, since you rooted out the problem entirely!