Skip to content

[firebase_messaging] Android: background service refactoring #261

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

Closed
wants to merge 19 commits into from

Conversation

ened
Copy link
Contributor

@ened ened commented Oct 3, 2019

Description

Currently, the FM plugin register 2 method channels (for FG and BG processing).
This breaks when a App consists of more than 1 standard isolate, as it means on every isolate instantiation, the plugin will overwrite the previous background channel.

By setting the background channel within the plugin registration, it also introduced a memory leak (see #260). More importantly, when the foreground App is stopped, notifications may not always come through, as the background channel has been overwritten.

Update 2020/06/07:
The PR has been updated since and now uses a FlutterEngine object internally which removes most of the previously needed integration work on Android. The need to register a plugin registrant callback has been removed.
Additionally, analysis with LeakCanary has been done and memory leaks removed.

Related Issues

This PR is based on #259; Merging that first is preferred to declutter this PR a bit.

#260

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • If the pull request affects only one plugin, the PR title starts with the name of the plugin in brackets (e.g. [cloud_firestore])
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

/cc @kroikie

@nhwilly
Copy link

nhwilly commented Oct 3, 2019

On behalf of those of us that don't have the skills to troubleshoot at this level, a sincere thanks.

Great to see these problems identified and addressed in rapid succession.

@ened
Copy link
Contributor Author

ened commented Oct 15, 2019

/cc @bparrishMines

@goody-h
Copy link

goody-h commented Nov 14, 2019

Glad to see this is a work in progress

@heaather
Copy link

heaather commented Nov 14, 2019 via email

@ened
Copy link
Contributor Author

ened commented Apr 30, 2020

The PR has been refactored and includes a few more simplifications on the Android side, including:

  • Remove the need to register things in the host application by switching to a FlutterEngine internally
  • Remove all static variables, so that the host Activity won't leak
  • Remove further variables as needed

I'd appreciate if the community could help and test this PR.

If you are interested, please adjust the dependency_overrides field in your pubspec.yaml and add the following:

  firebase_messaging:
    git:
      url: https://github.com/ened/flutterfire.git
      path: packages/firebase_messaging
      ref: 254e43d18ba0862d9406fa0d872689d1961b3025

@ened ened changed the title WIP [firebase_messaging] Move background channel listener location [firebase_messaging] Background Service refactoring Apr 30, 2020
@ened ened changed the title [firebase_messaging] Background Service refactoring [firebase_messaging] Android: background service refactoring Apr 30, 2020
@ened
Copy link
Contributor Author

ened commented Jun 8, 2020

@invertase - similarly, this PR could use someones attention. We are using it in production as else wise we'd run into memory leaks or similar. Also, it simplifies the integration on App side quite significantly.

@ened ened requested review from Ehesp and Salakar as code owners September 9, 2020 07:17
@ened ened force-pushed the firebase_messaging_background branch from 63e98ce to 95782bd Compare September 9, 2020 07:33
@Salakar
Copy link
Member

Salakar commented Sep 9, 2020

Thanks for this PR, its on our radar don't worry 🙃 we're close to shipping Functions & Storage reworks, after which we're focusing on messaging

@Salakar Salakar self-assigned this Sep 9, 2020
@Salakar
Copy link
Member

Salakar commented Sep 9, 2020

Also if you have any other PRs or issues that you're tracking and think we should look at please let us know below, thanks

@ened ened closed this Oct 4, 2020
Ehesp pushed a commit that referenced this pull request Oct 6, 2020
@firebase firebase locked and limited conversation to collaborators Nov 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants