-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Capture JSON RPC errors that presently get swallowed #31584
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
Conversation
| } | ||
|
|
||
| void _unhandledJsonRpcError(dynamic error, dynamic stack) { | ||
|
|
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.
Is this intentionally blank?
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.
No, forgot to save. Will fix tonight
|
@jonahwilliams any idea on the builderOptions failures? Do we need to pin build_runner? |
|
We can pin it for now and I can handle rolling the changes |
| const Map<String, String> _kManuallyPinnedDependencies = <String, String>{ | ||
| // Add pinned packages here. | ||
| 'flutter_gallery_assets': '0.1.8', // See //examples/flutter_gallery/pubspec.yaml | ||
| 'build_modules': '1.0.9', // TODO(jonahwilliams): migrate https://cirrus-ci.com/task/5747941259083776 |
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.
Thanks!
jonahwilliams
left a comment
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.
LGTM
|
This spiked our tech debt benchmark for package deps, but didn't add any new deps... |
|
It's because of the pinned dependency. That will go back down once we unpin the dep. |
Description
json_rpc_2 swallows exceptions. This can hide bugs in our callbacks, e.g. type conversion issues, which can result in an RPC call not doing what is expected.
This change hooks into a new parameter available that informs the caller of unhandled exceptions. For now, we just print them and assert(false). For Fuchsia debug protocol, we only print them.
Related Issues
Fixes #28531
Tests
The tests for this functionality exist upstream in the PR I wrote for json_rpc_2.
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.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?