[flutter_tools] tool exit after repeated network failure #64878
Conversation
flutter-dashboard
bot
added
the
tool
|
Possibly #52437 (comment) too |
| @@ -70,7 +70,7 @@ class Net { | |||
|
|
|||
| 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.
| @@ -1469,6 +1469,14 @@ class ArtifactUpdater { | |||
| 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.
| } | ||
| assert(response != null); | ||
|
|
||
| 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?
| @@ -120,7 +121,7 @@ class Net { | |||
| exitCode: kNetworkProblemExitCode,); | |||
| } | |||
| _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) |
|
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.
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