Retry devfs uploads in case they fail.#41406

Merged

aam merged 3 commits intoflutter:masterfrom

aam:retry-devfs-upload

Sep 30, 2019

Member

aam commented Sep 26, 2019

Fixes #34959.


          Retry devfs uploads in case they fail.

256add0

Fixes flutter#34959.

Contributor

fluttergithubbot commented Sep 26, 2019

It looks like this pull request may not have tests. Please make sure to add tests before merging. While there are exceptions to this rule, if this patch modifies code it is probably not an exception.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

fluttergithubbot added the tool label

googlebot added the cla: yes label

codecov bot commented Sep 26, 2019 •

edited

Loading

Codecov Report

Merging #41406 into master will increase coverage by 0.99%.
The diff coverage is 87.5%.

@@            Coverage Diff             @@
##           master   #41406      +/-   ##
==========================================
+ Coverage   59.43%   60.42%   +0.99%     
==========================================
  Files         193      193              
  Lines       18758    18755       -3     
==========================================
+ Hits        11149    11333     +184     
+ Misses       7609     7422     -187

Flag	Coverage Δ
#flutter_tool	`60.42% <87.5%> (+0.99%)`	⬆️

Impacted Files	Coverage Δ
packages/flutter_tools/lib/src/devfs.dart	`69.9% <87.5%> (+1.09%)`	⬆️
...s/flutter_tools/lib/src/commands/build_bundle.dart	`86.53% <0%> (-11.54%)`	⬇️
packages/flutter_tools/lib/src/run_cold.dart	`9.75% <0%> (-9.76%)`	⬇️
...ges/flutter_tools/lib/src/build_runner/web_fs.dart	`21.69% <0%> (-2.93%)`	⬇️
packages/flutter_tools/lib/src/build_info.dart	`75.13% <0%> (-1.63%)`	⬇️
packages/flutter_tools/lib/src/version.dart	`91.9% <0%> (-1.43%)`	⬇️
packages/flutter_tools/lib/src/cache.dart	`45.31% <0%> (-0.67%)`	⬇️
packages/flutter_tools/lib/src/commands/run.dart	`64.39% <0%> (-0.49%)`	⬇️
packages/flutter_tools/lib/src/vmservice.dart	`40.56% <0%> (-0.34%)`	⬇️
packages/flutter_tools/lib/src/project.dart	`84.76% <0%> (-0.05%)`	⬇️
... and 9 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 0980fa2...c1d0550. Read the comment docs.


          Add retry test

57d252d

aam requested a review from jonahwilliams

September 29, 2019 21:30

jonahwilliams approved these changes

View reviewed changes

Contributor

jonahwilliams left a comment

LGTM!

Thanks for doing all of the work to track this issue down and fix it. I just have a few style nits but otherwise looks good to go.

packages/flutter_tools/test/general.shard/devfs_test.dart Outdated

+                    final MockHttpClientResponse httpClientResponse = MockHttpClientResponse();
+                    int nRequest = 0;
+                    const int FAILED_ATTEMPTS = 5;
+                    when(httpRequest.close()).thenAnswer((_) {

Contributor

jonahwilliams Sep 29, 2019

nit: I'm not sure if the always_specify_types lint will accept _ but it probably shouldn't.

Suggested change

      
                  when(httpRequest.close()).thenAnswer((_) {
          
                  when(httpRequest.close()).thenAnswer((Invocation invocation) {

Member Author

aam Sep 30, 2019

Done

packages/flutter_tools/test/general.shard/devfs_test.dart Outdated

+                    when(httpClient.putUrl(any)).thenAnswer((_) => Future<HttpClientRequest>.value(httpRequest));
+                    final MockHttpClientResponse httpClientResponse = MockHttpClientResponse();
+                    int nRequest = 0;
+                    const int FAILED_ATTEMPTS = 5;

Contributor

jonahwilliams Sep 29, 2019

nit: no screaming caps

Suggested change

      
                  const int FAILED_ATTEMPTS = 5;
          
                  const int kFailedAttempts = 5;

Member Author

aam Sep 30, 2019

Done

packages/flutter_tools/test/general.shard/devfs_test.dart Outdated

+                    int nRequest = 0;
+                    const int FAILED_ATTEMPTS = 5;
+                    when(httpRequest.close()).thenAnswer((_) {
+                      if (nRequest++ < FAILED_ATTEMPTS) {

Contributor

jonahwilliams Sep 29, 2019

Suggested change

      
                    if (nRequest++ < FAILED_ATTEMPTS) {
          
                    if (nRequest++ < kFailedAttempts) {

Member Author

aam Sep 30, 2019

Done

packages/flutter_tools/test/general.shard/devfs_test.dart Outdated

+                    expect(report.syncedBytes, 22);
+                    expect(report.success, isTrue);
+                    verify(httpClient.putUrl(any)).called(FAILED_ATTEMPTS + 1);

Contributor

jonahwilliams Sep 29, 2019

Suggested change

      
                  verify(httpClient.putUrl(any)).called(FAILED_ATTEMPTS + 1);
          
                  verify(httpClient.putUrl(any)).called(kFailedAttempts + 1);

Member Author

aam Sep 30, 2019

Done

packages/flutter_tools/test/general.shard/devfs_test.dart Outdated

+                    expect(report.syncedBytes, 22);
+                    expect(report.success, isTrue);
+                    verify(httpClient.putUrl(any)).called(FAILED_ATTEMPTS + 1);
+                    verify(httpRequest.close()).called(FAILED_ATTEMPTS + 1);

Contributor

jonahwilliams Sep 29, 2019

Suggested change

      
                  verify(httpRequest.close()).called(FAILED_ATTEMPTS + 1);
          
                  verify(httpRequest.close()).called(kFailedAttempts + 1);

Member Author

aam Sep 30, 2019

Done

packages/flutter_tools/lib/src/devfs.dart Outdated

                     final Uri deviceUri = _outstanding.keys.first;
                     final DevFSContent content = _outstanding.remove(deviceUri);
-                    _startWrite(deviceUri, content);
+                    _startWrite(deviceUri, content, /* retry= */ 10);

Contributor

jonahwilliams Sep 29, 2019

I would update this to a named argument instead of using the comment syntax.

Suggested change

      
                  _startWrite(deviceUri, content, /* retry= */ 10);
          
                  _startWrite(deviceUri, content, retry: 10);

Member Author

aam Sep 30, 2019

Done

packages/flutter_tools/lib/src/devfs.dart Outdated

		@@ -302,19 +302,29 @@ class _DevFSHttpWriter {
		DevFSContent content, [

Contributor

jonahwilliams Sep 29, 2019

Suggested change

      
                DevFSContent content, [
          
                 DevFSContent content, {
          
                   int retry = 0
          
                 }

Member Author

aam Sep 30, 2019

Done

packages/flutter_tools/lib/src/devfs.dart Outdated

+                    } catch (error, trace) {
+                      if (!_completer.isCompleted) {
+                        printTrace('Error writing "$deviceUri" to DevFS: $error');
+                        if (retry-- > 0) {

Contributor

jonahwilliams Sep 29, 2019

pre/post increment is a bit weird inline with a >, kind of looks like a giant arrow. I think it would be easier to read if this were split into two lines

Member Author

aam Sep 30, 2019

Done


          Cleanup

c1d0550

jonahwilliams approved these changes

View reviewed changes

Contributor

jonahwilliams left a comment

Thanks!

aam merged commit 839fdbd into flutter:master

aam deleted the retry-devfs-upload branch

September 30, 2019 04:32

Member Author

aam commented Sep 30, 2019

Thanks!

Glad to help!

Contributor

tvolkert commented Sep 30, 2019

Do we know why the devfs writes were failing?

Member Author

aam commented Sep 30, 2019

Do we know why the devfs writes were failing?

Only hypothesis is that iproxy that we use to bridge from mac to http server running on the ios device decides to abandon http connection, which is always allowed. Unfortunately it didn't print anything in the logs to explain the reasons.

Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request


          Retry devfs uploads in case they fail. (flutter#41406)

ac493a5

* Retry devfs uploads in case they fail.

Fixes flutter#34959.

jonahwilliams mentioned this pull request

Error: Could not update files on device: OS Error: Connection reset by peer, errno = 54 #35989

Closed

jmagman mentioned this pull request

"Could not update files on device: OS Error: Connection reset by peer, errno = 54" iOS9 and iOS13 at least #43617

Closed

github-actions bot locked as resolved and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

tool