Skip to content

[flutter_tool] Use a timeout for xcode showBuildSettings - #39280

Merged
zanderso merged 1 commit into
flutter:masterfrom
zanderso:async-show-build-settings
Aug 28, 2019
Merged

[flutter_tool] Use a timeout for xcode showBuildSettings#39280
zanderso merged 1 commit into
flutter:masterfrom
zanderso:async-show-build-settings

Conversation

@zanderso

@zanderso zanderso commented Aug 26, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Description

Subcommands called by the tool may hang. Subsequent invocations may then succeed. This PR adds optional named parameters to runAsync() and runCheckedAsync() to provide for the need to retry subcommands that time-out.

In particular, when xcode runs with the -showBuildSettings flag it may timeout and then succeed on retry. This PR updates the call site from the issue reported below to time out after one minute, and retry one time. Additionally, this PR caches the result of the -showBuildSettings subcommand that could not be made asynchronous since it is called from a constructor.

Related Issues

#35988
#39141

Tests

I added the following tests:

Added tests of timeout with retry to process_test.dart. Updated other affected tests.

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 26, 2019
@codecov

codecov Bot commented Aug 26, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #39280 into master will increase coverage by 0.71%.
The diff coverage is 98.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #39280      +/-   ##
==========================================
+ Coverage   55.99%   56.71%   +0.71%     
==========================================
  Files         195      195              
  Lines       18428    18469      +41     
==========================================
+ Hits        10319    10474     +155     
+ Misses       8109     7995     -114
Flag Coverage Δ
#flutter_tool 56.71% <98.38%> (+0.71%) ⬆️
Impacted Files Coverage Δ
packages/flutter_tools/lib/src/plugins.dart 82.45% <100%> (ø) ⬆️
packages/flutter_tools/lib/src/project.dart 83.7% <100%> (+0.48%) ⬆️
packages/flutter_tools/lib/src/base/process.dart 85.8% <100%> (+3.63%) ⬆️
...ackages/flutter_tools/lib/src/macos/cocoapods.dart 86.11% <100%> (+0.12%) ⬆️
packages/flutter_tools/lib/src/ios/xcodeproj.dart 87.8% <88.88%> (+2.64%) ⬆️
packages/flutter_tools/lib/src/base/net.dart 36.17% <0%> (-48.94%) ⬇️
packages/flutter_tools/lib/src/base/common.dart 72.72% <0%> (-18.19%) ⬇️
packages/flutter_tools/lib/src/version.dart 90.9% <0%> (-1.92%) ⬇️
packages/flutter_tools/lib/src/cache.dart 43.02% <0%> (-0.73%) ⬇️
packages/flutter_tools/lib/src/base/logger.dart 82.06% <0%> (-0.39%) ⬇️
... and 10 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 91ae140...0f8bb60. Read the comment docs.

christopherfujino
christopherfujino approved these changes Aug 28, 2019
View reviewed changes

@christopherfujino christopherfujino left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

jonahwilliams
jonahwilliams approved these changes Aug 28, 2019
View reviewed changes

@jonahwilliams jonahwilliams left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@zanderso
zanderso merged commit b7c714e into flutter:master Aug 28, 2019
@zanderso
zanderso deleted the async-show-build-settings branch August 28, 2019 17:03
This was referenced Aug 28, 2019
Closed
Merged
@Hixie

Hixie commented Oct 8, 2019

Copy link
Copy Markdown
Contributor

This violates our style guide which says to never have timeouts because they are a source of flakes.

Are these processes destructive or idempotent? If they're idempotent, then I suggest that we change this to not kill the first process, but to show a message after a few seconds saying "This seems to be taking longer than usual" and then start a second process in parallel, to see if it returns faster, and if it does, kill the first one. If the second takes more than a minute, kill it then try a third one, etc, leaving still the first one running, in case it's just being super slow.

@Hixie

Hixie commented Oct 8, 2019

Copy link
Copy Markdown
Contributor

Filed as #42265

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@christopherfujino christopherfujino christopherfujino approved these changes
@jonahwilliams jonahwilliams jonahwilliams approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@zanderso @Hixie @christopherfujino @jonahwilliams @googlebot