[ Tool ] Only process a single unhandled tool exception #178335
Conversation
Without this change, if multiple asynchronous exceptions are thrown while processing an exception, the shutdown hooks can be run multiple times and multiple exception analytics events can be sent for a single process crash, skewing crashalytics data. Fixes #178318
github-actions
bot
added
the
tool
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to ensure that unhandled tool exceptions are processed only once, preventing duplicate shutdown hook executions and analytics events. This is achieved by introducing a _alreadyHandlingToolError future that acts as a guard. The implementation is clean and effectively solves the described problem. A new test case has been added to verify this fix by simulating multiple asynchronous exceptions and asserting that only a single analytics event is sent. My review includes a couple of minor suggestions in the new test code to improve its clarity and maintainability.
packages/flutter_tools/test/general.shard/runner/runner_test.dart
Outdated
Show resolved
Hide resolved
| // This flutterVersion disables crash reporting. | ||
| flutterVersion: '[user-branch]/', | ||
| reportCrashes: true, | ||
| shutdownHooks: ShutdownHooks(), |
There was a problem hiding this comment.
Can we verify shutdown hooks are only called once?
There was a problem hiding this comment.
Looking at the ShutdownHooks implementation, we actually guard against running the hooks more than once as it's possible to invoke exitWithHooks after returning from a command and for it to be invoked due to an outstanding asynchronous event that throws.
I've updated the PR description to reflect this. The main thing we're testing here is that we don't try and sent multiple events due to asynchronous exceptions being thrown in quick succession. I've verified this test fails without the fix.
|
Are you at all worried about the loss of data by throwing the first tool error we see and none of the rest? |
No, I don't think so. Errors thrown after the initial error are likely due to a cascading failure, and we'd have to explicitly check for multiple reports (possibly with different stack traces) from an individual client within a short timeframe to even identify if multiple exceptions were thrown during a crash. |
flutter-dashboard
bot
removed
the
autosubmit
bkonyi
added
cp: beta
Without this change, if multiple asynchronous exceptions are thrown while processing an exception, multiple exception analytics events can be sent for a single process crash, skewing crashlytics data. Fixes flutter#178318
Without this change, if multiple asynchronous exceptions are thrown while processing an exception, multiple exception analytics events can be sent for a single process crash, skewing crashlytics data. Fixes flutter#178318
Without this change, if multiple asynchronous exceptions are thrown while processing an exception, multiple exception analytics events can be sent for a single process crash, skewing crashlytics data. Fixes flutter#178318
Without this change, if multiple asynchronous exceptions are thrown while processing an exception, multiple exception analytics events can be sent for a single process crash, skewing crashlytics data. Fixes flutter#178318
Without this change, if multiple asynchronous exceptions are thrown while processing an exception, multiple exception analytics events can be sent for a single process crash, skewing crashlytics data. Fixes flutter#178318
3 participants
Without this change, if multiple asynchronous exceptions are thrown while processing an exception, multiple exception analytics events can be sent for a single process crash, skewing crashlytics data.
Fixes #178318