Repository navigation
[flutter_tools] tool exit after repeated network failure - #64878
Conversation
flutter-dashboard
Bot
added
the
tool
|
Possibly #52437 (comment) too |
| if (maxAttempts != null && attempts >= maxAttempts) { | ||
| _logger.printStatus('Download failed -- retry $attempts'); | ||
| return null; | ||
| throw Exception('Failed to download $url'); |
There was a problem hiding this comment.
_attempt could return null on success, and the exception object on an error. Then here you can rethrow the most recent error. Then at the callsite, you can catch IOException, or something more narrow, since it doesn't look like the text 'Failed to download...' is used there. The caller also presumably already knows what the url is.
| try { | ||
| _ensureExists(tempFile.parent); | ||
| await _net.fetchUrl(url, destFile: tempFile, maxAttempts: 2); | ||
| } on Exception { |
There was a problem hiding this comment.
Ehhhh. Okay, sorry. Maybe that wasn't the best suggestion. If there's some way to avoid catching Exception here that would be good, though, since it's only the IOExceptions that probably indicate a network issue on the end-user's side.
|
|
||
| if (response.statusCode != HttpStatus.ok) { | ||
| _logger.printTrace('Download error: ${response.statusCode} ${response.reasonPhrase}'); | ||
| final Exception exception = Exception( |
There was a problem hiding this comment.
Where did the throwTooleExit go?
| } | ||
| _logger.printError(error.toString()); | ||
| rethrow; | ||
| return error; |
There was a problem hiding this comment.
Maybe these should all rethrow, fetchUrl can do a try {} catch on IOException {} with a continue in the catch if there are still attempts left? doesRemoteFileExist would also need a try {} catch {}, too, then I guess.
|
I think the net logic is quite convoluted actually, which is part of the issue with trying to update it. It's not really clear from the contract what sorts of exceptions it should be handling, and which ones to leave to the callers. If I were going to spend some time on it, what I would do is more or less replace This would let the artifact updater contain the logic for writing the bytes, which would allow it to handle any IOexceptions from writing. |
|
(also removing the retry logic, to allow the callers to include that if they wanted to) |
zanderso
left a comment
zanderso
left a comment
There was a problem hiding this comment.
Were there any other users of Net?
Should the number of retries be wired up to method arguments or command line flags of e.g. precache?
|
There is some usage in create.dart (for snippet creation), update_packages for downloading coverage (which doesn't need to be that resilient), and then the cache check that artifacts exist. As a follow up these could be refactored/removed. Likely we'll want to keep the cache check for existence (header only request) and find out how to remove the others. |
|
I could hoist the retry count into a constant for now, and we could make it configurable if there is a feature request to do so |
Exit the tool after a repeated network error to download. previously we were returning null and continuing on, leading to a ProcessException when we unzipped a missing file.
3 participants
Description
Exit the tool after a repeated network error to download. previously we were returning null and continuing on, leading to a ProcessException when we unzipped a missing file.
Fixes #64876
Fixes #64857