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

[flutter_tool] Only send one crash report per run #38925

Merged
merged 1 commit into from Aug 21, 2019
Merged

[flutter_tool] Only send one crash report per run #38925

merged 1 commit into from Aug 21, 2019

Conversation

zanderso
Copy link
Member

@zanderso zanderso commented Aug 20, 2019

Description

Prior to this PR, if there are multiple unhandled exceptions in a single invocation of the tool, they will all be sent to crash reporting. This is problematic when a stream with lots of data has a listener callback that throws a synchronous exception. The same 'crash' will generate potentially hundreds of reports until the event loop turns enough times for the tool to exit.

This PR ensures that only the first unhandled exception will be sent to crash reporting. Subsequent unhandled exceptions will still be logged to the console and/or a crash log file.

Related Issues

Noise/duplicates in crash reporting.

Tests

I added the following tests:

New test in crash_reporting_test.dart.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read [Handling breaking changes]). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

@zanderso zanderso added tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Aug 20, 2019
Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

I wonder if this explains the relatively high volume/low users of the frontend_server crash?

time.elapse(const Duration(seconds: 1));
time.flushMicrotasks();
});
expect(await exitCodeCompleter.future, equals(1));
Copy link
Member

Choose a reason for hiding this comment

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

nit: I would use expect(await foo, 1)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it was the cause of the profile we are seeing from the frontend server crashers. I cycled through the error messages for a master version, and there was a discernible pattern.

@codecov
Copy link

codecov bot commented Aug 21, 2019

Codecov Report

Merging #38925 into master will decrease coverage by 0.82%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #38925      +/-   ##
==========================================
- Coverage   56.26%   55.44%   -0.83%     
==========================================
  Files         195      195              
  Lines       18314    18316       +2     
==========================================
- Hits        10305    10155     -150     
- Misses       8009     8161     +152
Flag Coverage Δ
#flutter_tool 55.44% <85.71%> (-0.83%) ⬇️
Impacted Files Coverage Δ
...utter_tools/lib/src/reporting/crash_reporting.dart 76.19% <100%> (+1.19%) ⬆️
packages/flutter_tools/lib/runner.dart 71.11% <83.33%> (ø) ⬆️
...ackages/flutter_tools/lib/src/proxy_validator.dart 21.05% <0%> (-73.69%) ⬇️
packages/flutter_tools/lib/src/commands/drive.dart 5.83% <0%> (-69.17%) ⬇️
...ckages/flutter_tools/lib/src/commands/devices.dart 53.84% <0%> (-23.08%) ⬇️
...r_tools/lib/src/runner/flutter_command_runner.dart 56.19% <0%> (-13.23%) ⬇️
packages/flutter_tools/lib/src/base/context.dart 49.12% <0%> (-8.78%) ⬇️
packages/flutter_tools/lib/src/version.dart 86.89% <0%> (-5.83%) ⬇️
...ges/flutter_tools/lib/src/build_system/source.dart 81.69% <0%> (-4.23%) ⬇️
packages/flutter_tools/lib/src/device.dart 54.81% <0%> (-2.41%) ⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22db62c...ca720bb. Read the comment docs.

@zanderso zanderso merged commit 36e8b93 into flutter:master Aug 21, 2019
@zanderso zanderso deleted the only-send-one-crash branch August 21, 2019 20:07
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants