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] Don't crash on Android emulator startup failure #48995

Merged
merged 1 commit into from
Jan 17, 2020
Merged

[flutter_tool] Don't crash on Android emulator startup failure #48995

merged 1 commit into from
Jan 17, 2020

Conversation

zanderso
Copy link
Member

@zanderso zanderso commented Jan 16, 2020

Description

Before my refactoring of ProcessUtils, when the Android emulator process failed, we would throw and catch the error as a string and print it. After my refactoring of ProcessUtils the error was thrown as a ProcessException but the code to catch the exception wasn't updated. This resulted in the Android emulator process failures crashing the tool instead of being ToolExits as was intended.

Only the Android emulator code was using the String based throw/catch. This PR removes that, and refactors the emulator process management code to print errors instead of relying on exceptions.

Related Issues

Current top crasher on stable from crash logging.

Tests

I added the following tests:

Added tests to android_emulator_test.dart.

Breaking Change

Did any tests fail when you ran them? Please read [Handling breaking changes].

  • No, no existing tests failed, so 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 Jan 16, 2020
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

unawaited(stdoutSubscription.cancel());
unawaited(stderrSubscription.cancel());
if (!earlyFailure && stderrList.isEmpty) {
return;
Copy link

Choose a reason for hiding this comment

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

would it make sense to print message in this case as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I shuffled the prints around a bit, so that it'll be a bit more clear.

Copy link

@blasten blasten left a comment

Choose a reason for hiding this comment

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

Lgtm, thanks!

@fluttergithubbot fluttergithubbot merged commit e8222aa into flutter:master Jan 17, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 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.

5 participants