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

RollbarFlutter.run is limiting (and probably not working as intended) #90

Open
rfuerst87 opened this issue Jan 10, 2023 · 17 comments
Open
Assignees
Labels
enhancement New feature or request

Comments

@rfuerst87
Copy link

rfuerst87 commented Jan 10, 2023

The current design of RollbarFlutter makes it impossible to integrate it with custom error handlers. Especially FlutterRollbar.run is very limiting. IMHO it has several issues:

  1. RollbarFlutter overwrites FlutterError.onError without any warnings. Having a custom onError function is not compatible with RollbarFlutter.
  2. There is no way to just initialize Rollbar without running an app. This basically makes RollbarFlutter incompatible with any other error handler.
  3. RollbarFlutter is creating it's own guarded zone without the possibility to bubble up errors. FlutterRollbar swallows all the errors without giving other error handlers a chance to catch an error.
  4. WidgetsFlutterBinding.ensureInitialized() is called in the wrong place. When initializing a guarded zone WidgetsFlutterBinding.ensureInitialized() must be called within that zone. Calling it outside (as it is done in run) causes the guarded zone to not catch errors. At this point I think catching async errors does not properly work. Said that, with the current design one has to ensure WidgetsFlutterBinding.ensureInitialized() is not called manually before running RollbarFlutter.run.

In my eyes the design of RollbarFlutter.run is flawed. While the intention to make it as easy to integrate as possible is good, but it also bears the downside of inflexibility. Instead RollbarFlutter should expose an API to initialize Rollbar and let the user decide how to implement. Something like this could work for example:

void main() async {
 
  runZonedGuarded(() async {
    // let the user be responsible for the guarded zone to 
    // properly run WidgetsFlutterBinding.ensureInitialized()
    WidgetsFlutterBinding.ensureInitialized();

    // Initialize rollbar
    await RollbarFlutter.initialize();
    
    // Give the users a chance to implement their own onError
    FlutterError.onError = RollbarFlutter.onError;

    runApp(MyApp());
  }, (exception, stackTrace) {
    // Also give users a chance to do anything else with async errors
    RollbarFlutter.error(exception, stackTrace));
  }
}

For convenience one could provide simple helper methods in RollbarFlutter to make this an one-liner again. I'm not sure whether this is needed to be honest.

Please let me know what you think. I'd offer my support here to come up with an improved version. We are currently forced to work with a fork which I'd get rid of better sooner than later.

@ghost
Copy link

ghost commented Feb 9, 2023

Hey @rfuerst87, I appreciate that you shared your concerns in a very detailed manner. I'll talk to the dev team about your comments.

@matux matux self-assigned this Mar 15, 2023
@matux matux added the enhancement New feature or request label Mar 15, 2023
@mazen930
Copy link

Any updates here ? , as this become more and more serious since i moved to use 3.10 flutter sdk , it throws random errors and not stable

@mazen930
Copy link

@rfuerst87 is there any work around here ?

@ghost
Copy link

ghost commented May 30, 2023

Hey @mazen930, Thanks for reporting this issue. Can you share what errors Flutter throws? Do you use the latest Rollbar Flutter SDK (v1.3.0)?

@mrchenmo
Copy link

Hey @mazen930, Thanks for reporting this issue. Can you share what errors Flutter throws? Do you use the latest Rollbar Flutter SDK (v1.3.0)?

The following assertion was thrown during runApp:
Zone mismatch.

The Flutter bindings were initialized in a different zone than is now being used. This will likely cause confusion and bugs as any zone-specific configuration will inconsistently use the configuration of the original binding initialization zone or this zone based on hard-to-predict factors such as which zone was active when a particular callback was set.
It is important to use the same zone when calling ensureInitialized on the binding as when calling runApp later.
To make this warning fatal, set BindingBase.debugZoneErrorsAreFatal to true before the bindings are initialized (i.e. as the first statement in void main() { }).
When the exception was thrown, this was the stack:
#0 BindingBase.debugCheckZone. (package:flutter/src/foundation/binding.dart:497:29)
#1 BindingBase.debugCheckZone (package:flutter/src/foundation/binding.dart:502:6)
#2 runApp (package:flutter/src/widgets/binding.dart:1080:18)
#3 main. (package:flutter_app/main.dart:22:42)
#4 RollbarFlutter.run. (package:rollbar_flutter/src/rollbar.dart:53:22)

#5 RollbarFlutter.run (package:rollbar_flutter/src/rollbar.dart:47:5)

#6 main (package:flutter_app/main.dart:22:3)

@mrchenmo
Copy link

The 3.10 version of the SDK restricts the call of ensureInitialized, which must be called after runApp to ensure that it runs in the same Zone as the app. In the code of rollbar, the ensureInitialized method is called before runApp

@ghost
Copy link

ghost commented May 31, 2023

Got it, thanks for the clarification. Let me take this back to the developers.

@matux
Copy link
Collaborator

matux commented May 31, 2023

I can reproduce, trying to find a solution.

@mrchenmo There's no need to ensure bindings are initialized after runApp() since runApp() initializes the bindings. So, it must be done before runApp(). Otherwise, we get this error:

VERBOSE-2:dart_vm_initializer.cc(41)] Unhandled Exception: Binding has not yet been initialized.
The "instance" getter on the ServicesBinding binding mixin is only available once that binding has been initialized.
Typically, this is done by calling "WidgetsFlutterBinding.ensureInitialized()" or "runApp()" (the latter calls the former). Typically this call is done in the "void main()" method. The "ensureInitialized" method is idempotent; calling it multiple times is not harmful. After calling that method, the "instance" getter will return the binding.
In a test, one can call "TestWidgetsFlutterBinding.ensureInitialized()" as the first line in the test's "main()" method to initialize the binding.
If ServicesBinding is a custom binding mixin, there must also be a custom binding class, like WidgetsFlutterBinding, but that mixes in the selected binding, and that is the class that must be constructed before using the "instance" getter.
#0      BindingBase.checkInstance.<anonymous closure> (package:flutter<…>

The idea behind calling ensureInitialized is to make sure the bindings are initialized before running Rollbar because we need the ServicesBindings to connect to the platform.

The problem is that we have two code paths for when users disable error capturing (which doesn't create a Zone in which to run Rollbar), and the normal mode (with error capturing) where we do create a zone.

I believe I was able to fix it. Will post a PR soon and link it here.

@matux
Copy link
Collaborator

matux commented May 31, 2023

With regards to @rfuerst87, I know it's been quite a few months, but I believe your changes to be worth implementing.

I wouldn't want to break userspace if someone decides to upgrade from 1.3.0 to 1.4.0, so I'd approach this as providing an alternative (more advanced) way of initializing Rollbar Flutter, together with the usual, simple one.

What do you think? If you've already forgotten about this, no problem, I will implement your suggestions one way or another.

@rfuerst87
Copy link
Author

I guess it's important to mention that starting from Flutter 3.3 you souldn't use runZonedGuarded to catch errors in flutter anymore. Instead PlatformDispatcher.onError is now the recommended approach.

See What's new in Flutter 3.3 and Handling Errors in Flutter docs .

@matux
Copy link
Collaborator

matux commented Jun 2, 2023

I guess it's important to mention that starting from Flutter 3.3 you souldn't use runZonedGuarded to catch errors in flutter anymore. Instead PlatformDispatcher.onError is now the recommended approach.

See What's new in Flutter 3.3 and Handling Errors in Flutter docs .

I made this into a new issue @ #101, I appreciate the advice!

@matux
Copy link
Collaborator

matux commented Jun 2, 2023

@mazen930 @mrchenmo The update with the bug fix (1.3.1) is now published @ https://pub.dev/packages/rollbar_flutter.

@mazen930
Copy link

mazen930 commented Jun 4, 2023

@rollbar-bborsits the error is as @mrchenmo mention above

@mazen930
Copy link

mazen930 commented Jun 4, 2023

@matux unfortunately it didn't work (same error exist) , do i need to edit anything rather than just update ?

@matux
Copy link
Collaborator

matux commented Jun 4, 2023

@mazen930 no, you shouldn't have to make any changes. Could you confirm you have the latest version 1.3.1?

The SDK example used to crash with this error and is working fine now, so I suspect you might not be using the latest.

@mazen930
Copy link

mazen930 commented Jun 6, 2023

I can ensure that it's latest update :C

@Nico04
Copy link

Nico04 commented Oct 25, 2023

Any update on this?
In my case I'd like to debugPrint each error but it's swallowed by the run command.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants