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.

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

fix: Prevent error when using logging from background isolate #634

Merged

Conversation

fuzzybinary
Copy link
Collaborator

What and why?

When creating a logger from a background isolate, Flutter would throw because WidgetsFlutterBinding wasn't initialized. This is specifically because we were adding a callback handler, which is not supported from background isolates.

Add checks to both logs and RUM and prevent initialization of things we know won't work from background isolates.

This is not full support for background isolates but usable as a work around.

refs: #580

Review checklist

  • This pull request has appropriate unit and / or integration tests
  • This pull request references a Github or JIRA issue

When creating a logger from a background isolate, Flutter would throw because WidgetsFlutterBinding wasn't initialized. This is specifically because we were adding a callback handler, which is not supported from background isolates.

Add checks to both logs and RUM and prevent initialization of things we know won't work from background isolates.

This is not full support for background isolates but usable as a work around.

refs: #580
@@ -26,7 +26,9 @@ class DdRumMethodChannel extends DdRumPlatform {
internalLogger: core.internalLogger,
);

methodChannel.setMethodCallHandler(callbackHandler.handleMethodCall);
if (ServicesBinding.rootIsolateToken != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This potentially could cause issues when using multiple flutters - but I understand this is not fully supported.

I have zero clue if it actually will impact multiple flutters - I kinda think it WON'T simply because using two FlutterEngines here means we're not in a spawned isolate ever - but just wanted to mention it.

It might be worthwhile, if DD wants to support multiple flutter engines directly, to add a test / example based off of https://github.com/flutter/samples/tree/main/add_to_app/multiple_flutters.

Appreciate the effort here regardless! 🎉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the input. I'll see if I can run a quick test with multiple flutters, but my guess there is that each engine will have its own root Isolate and therefore its own rootIsolateToken.

@fuzzybinary fuzzybinary merged commit 3f37119 into develop Jul 30, 2024
7 checks passed
@fuzzybinary fuzzybinary deleted the jward/580-background-isolate-logging-workaround branch July 30, 2024 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants