Retry devfs uploads in case they fail. #41406
Conversation
|
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. |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
jonahwilliams
left a comment
There was a problem hiding this 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.
| final MockHttpClientResponse httpClientResponse = MockHttpClientResponse(); | ||
| int nRequest = 0; | ||
| const int FAILED_ATTEMPTS = 5; | ||
| when(httpRequest.close()).thenAnswer((_) { |
There was a problem hiding this comment.
nit: I'm not sure if the always_specify_types lint will accept _ but it probably shouldn't.
| when(httpRequest.close()).thenAnswer((_) { | |
| when(httpRequest.close()).thenAnswer((Invocation invocation) { |
| when(httpClient.putUrl(any)).thenAnswer((_) => Future<HttpClientRequest>.value(httpRequest)); | ||
| final MockHttpClientResponse httpClientResponse = MockHttpClientResponse(); | ||
| int nRequest = 0; | ||
| const int FAILED_ATTEMPTS = 5; |
There was a problem hiding this comment.
nit: no screaming caps
| const int FAILED_ATTEMPTS = 5; | |
| const int kFailedAttempts = 5; |
| int nRequest = 0; | ||
| const int FAILED_ATTEMPTS = 5; | ||
| when(httpRequest.close()).thenAnswer((_) { | ||
| if (nRequest++ < FAILED_ATTEMPTS) { |
There was a problem hiding this comment.
| if (nRequest++ < FAILED_ATTEMPTS) { | |
| if (nRequest++ < kFailedAttempts) { |
|
|
||
| expect(report.syncedBytes, 22); | ||
| expect(report.success, isTrue); | ||
| verify(httpClient.putUrl(any)).called(FAILED_ATTEMPTS + 1); |
There was a problem hiding this comment.
| verify(httpClient.putUrl(any)).called(FAILED_ATTEMPTS + 1); | |
| verify(httpClient.putUrl(any)).called(kFailedAttempts + 1); |
| expect(report.syncedBytes, 22); | ||
| expect(report.success, isTrue); | ||
| verify(httpClient.putUrl(any)).called(FAILED_ATTEMPTS + 1); | ||
| verify(httpRequest.close()).called(FAILED_ATTEMPTS + 1); |
There was a problem hiding this comment.
| verify(httpRequest.close()).called(FAILED_ATTEMPTS + 1); | |
| verify(httpRequest.close()).called(kFailedAttempts + 1); |
| final Uri deviceUri = _outstanding.keys.first; | ||
| final DevFSContent content = _outstanding.remove(deviceUri); | ||
| _startWrite(deviceUri, content); | ||
| _startWrite(deviceUri, content, /* retry= */ 10); |
There was a problem hiding this comment.
I would update this to a named argument instead of using the comment syntax.
| _startWrite(deviceUri, content, /* retry= */ 10); | |
| _startWrite(deviceUri, content, retry: 10); |
|
|
||
| Future<void> _startWrite( | ||
| Uri deviceUri, | ||
| DevFSContent content, [ |
There was a problem hiding this comment.
| DevFSContent content, [ | |
| DevFSContent content, { | |
| int retry = 0 | |
| } |
| } catch (error, trace) { | ||
| if (!_completer.isCompleted) { | ||
| printTrace('Error writing "$deviceUri" to DevFS: $error'); | ||
| if (retry-- > 0) { |
There was a problem hiding this comment.
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
Glad to help! |
|
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. |
* Retry devfs uploads in case they fail. Fixes flutter#34959.
Fixes #34959.