[flutter_tools] tool exit after repeated network failure (#64878)

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.

packages/flutter_tools/lib/src/cache.dart

@@ -9,7 +9,7 @@ import 'package:meta/meta.dart';
 import 'android/gradle_utils.dart';
 import 'base/common.dart';
 import 'base/file_system.dart';
-import 'base/io.dart' show ProcessException, SocketException;
+import 'base/io.dart' show HttpClient, HttpClientRequest, HttpClientResponse, HttpStatus, ProcessException, SocketException;
 import 'base/logger.dart';
 import 'base/net.dart';
 import 'base/os.dart' show OperatingSystemUtils;
@@ -144,10 +144,11 @@ class Cache {
   ArtifactUpdater _createUpdater() {
     return ArtifactUpdater(
       operatingSystemUtils: _osUtils,
-      net: _net,
       logger: _logger,
       fileSystem: _fileSystem,
       tempStorage: getDownloadDir(),
+      platform: _platform,
+      httpClient: HttpClient(),
     );
   }
 
@@ -1403,21 +1404,27 @@ const List<List<String>> _dartSdks = <List<String>> [
 class ArtifactUpdater {
   ArtifactUpdater({
     @required OperatingSystemUtils operatingSystemUtils,
-    @required Net net,
     @required Logger logger,
     @required FileSystem fileSystem,
-    @required Directory tempStorage
+    @required Directory tempStorage,
+    @required HttpClient httpClient,
+    @required Platform platform,
   }) : _operatingSystemUtils = operatingSystemUtils,
-       _net = net,
+       _httpClient = httpClient,
        _logger = logger,
        _fileSystem = fileSystem,
-       _tempStorage = tempStorage;
+       _tempStorage = tempStorage,
+       _platform = platform;
+
+  /// The number of times the artifact updater will repeat the artifact download loop.
+  static const int _kRetryCount = 2;
 
   final Logger _logger;
-  final Net _net;
   final OperatingSystemUtils _operatingSystemUtils;
   final FileSystem _fileSystem;
   final Directory _tempStorage;
+  final HttpClient _httpClient;
+  final Platform _platform;
 
   /// Keep track of the files we've downloaded for this execution so we
   /// can delete them after completion. We don't delete them right after
@@ -1463,12 +1470,38 @@ class ArtifactUpdater {
       message,
       timeout: null, // This will take a variable amount of time based on network connectivity.
     );
-    int retries = 2;
+    int retries = _kRetryCount;
 
     while (retries > 0) {
       try {
         _ensureExists(tempFile.parent);
-        await _net.fetchUrl(url, destFile: tempFile, maxAttempts: 2);
+        final IOSink ioSink = tempFile.openWrite();
+        await _download(url, ioSink);
+        await ioSink.close();
+      } on Exception catch (err) {
+        _logger.printTrace(err.toString());
+        retries -= 1;
+        if (retries == 0) {
+          throwToolExit(
+            'Failed to download $url. Ensure you have network connectivity and then try again.',
+          );
+        }
+        continue;
+      } on ArgumentError catch (error) {
+        final String overrideUrl = _platform.environment['FLUTTER_STORAGE_BASE_URL'];
+        if (overrideUrl != null && url.toString().contains(overrideUrl)) {
+          _logger.printError(error.toString());
+          throwToolExit(
+            'The value of FLUTTER_STORAGE_BASE_URL ($overrideUrl) could not be '
+            'parsed as a valid url. Please see https://flutter.dev/community/china '
+            'for an example of how to use it.\n'
+            'Full URL: $url',
+            exitCode: kNetworkProblemExitCode,
+          );
+        }
+        // This error should not be hit if there was not a storage URL override, allow the
+        // tool to crash.
+        rethrow;
       } finally {
         status.stop();
       }
@@ -1488,6 +1521,16 @@ class ArtifactUpdater {
     }
   }
 
+  /// Download bytes from [url], throwing non-200 responses as an exception.
+  Future<void> _download(Uri url, IOSink ioSink) async {
+    final HttpClientRequest request = await _httpClient.getUrl(url);
+    final HttpClientResponse response = await request.close();
+    if (response.statusCode != HttpStatus.ok) {
+      throw Exception(response.statusCode);
+    }
+    await response.forEach(ioSink.add);
+  }
+
   /// Create a temporary file and invoke [onTemporaryFile] with the file as
   /// argument, then add the temporary file to the [downloadedFiles].
   File _createDownloadFile(String name) {

packages/flutter_tools/test/general.shard/artifact_updater_test.dart

@@ -8,24 +8,26 @@ import 'package:file_testing/file_testing.dart';
 import 'package:flutter_tools/src/base/file_system.dart';
 import 'package:flutter_tools/src/base/io.dart';
 import 'package:flutter_tools/src/base/logger.dart';
-import 'package:flutter_tools/src/base/net.dart';
 import 'package:flutter_tools/src/base/os.dart';
+import 'package:flutter_tools/src/base/platform.dart';
 import 'package:flutter_tools/src/cache.dart';
 import 'package:mockito/mockito.dart';
 
 import '../src/common.dart';
 
+final Platform testPlatform = FakePlatform(environment: <String, String>{});
+
 void main() {
   testWithoutContext('ArtifactUpdater can download a zip archive', () async {
-    final FakeNet net = FakeNet();
     final MockOperatingSystemUtils operatingSystemUtils = MockOperatingSystemUtils();
     final MemoryFileSystem fileSystem = MemoryFileSystem.test();
     final BufferLogger logger = BufferLogger.test();
     final ArtifactUpdater artifactUpdater = ArtifactUpdater(
       fileSystem: fileSystem,
       logger: logger,
-      net: net,
       operatingSystemUtils: operatingSystemUtils,
+      platform: testPlatform,
+      httpClient: MockHttpClient(),
       tempStorage: fileSystem.currentDirectory.childDirectory('temp')
         ..createSync(),
     );
@@ -39,16 +41,101 @@ void main() {
     expect(fileSystem.file('out/test'), exists);
   });
 
+  testWithoutContext('ArtifactUpdater will re-attempt on a non-200 response', () async {
+    final MockOperatingSystemUtils operatingSystemUtils = MockOperatingSystemUtils();
+    final MemoryFileSystem fileSystem = MemoryFileSystem.test();
+    final BufferLogger logger = BufferLogger.test();
+    final MockHttpClient client = MockHttpClient();
+    client.testRequest.testResponse.statusCode = HttpStatus.preconditionFailed;
+    final ArtifactUpdater artifactUpdater = ArtifactUpdater(
+      fileSystem: fileSystem,
+      logger: logger,
+      operatingSystemUtils: operatingSystemUtils,
+      platform: testPlatform,
+      httpClient: client,
+      tempStorage: fileSystem.currentDirectory.childDirectory('temp')
+        ..createSync(),
+    );
+
+    await expectLater(() async => await artifactUpdater.downloadZipArchive(
+      'test message',
+      Uri.parse('http:///test.zip'),
+      fileSystem.currentDirectory.childDirectory('out'),
+    ), throwsToolExit());
+
+    expect(client.attempts, 2);
+    expect(logger.statusText, contains('test message'));
+    expect(fileSystem.file('out/test'), isNot(exists));
+  });
+
+  testWithoutContext('ArtifactUpdater will tool exit on an ArgumentError from http client with base url override', () async {
+    final MockOperatingSystemUtils operatingSystemUtils = MockOperatingSystemUtils();
+    final MemoryFileSystem fileSystem = MemoryFileSystem.test();
+    final BufferLogger logger = BufferLogger.test();
+    final MockHttpClient client = MockHttpClient();
+    client.argumentError = true;
+    final ArtifactUpdater artifactUpdater = ArtifactUpdater(
+      fileSystem: fileSystem,
+      logger: logger,
+      operatingSystemUtils: operatingSystemUtils,
+      platform: FakePlatform(
+        environment: <String, String>{
+          'FLUTTER_STORAGE_BASE_URL': 'foo-bar'
+        },
+      ),
+      httpClient: client,
+      tempStorage: fileSystem.currentDirectory.childDirectory('temp')
+        ..createSync(),
+    );
+
+    await expectLater(() async => await artifactUpdater.downloadZipArchive(
+      'test message',
+      Uri.parse('http:///foo-bar/test.zip'),
+      fileSystem.currentDirectory.childDirectory('out'),
+    ), throwsToolExit());
+
+    expect(client.attempts, 1);
+    expect(logger.statusText, contains('test message'));
+    expect(fileSystem.file('out/test'), isNot(exists));
+  });
+
+  testWithoutContext('ArtifactUpdater will rethrow on an ArgumentError from http client without base url override', () async {
+    final MockOperatingSystemUtils operatingSystemUtils = MockOperatingSystemUtils();
+    final MemoryFileSystem fileSystem = MemoryFileSystem.test();
+    final BufferLogger logger = BufferLogger.test();
+    final MockHttpClient client = MockHttpClient();
+    client.argumentError = true;
+    final ArtifactUpdater artifactUpdater = ArtifactUpdater(
+      fileSystem: fileSystem,
+      logger: logger,
+      operatingSystemUtils: operatingSystemUtils,
+      platform: testPlatform,
+      httpClient: client,
+      tempStorage: fileSystem.currentDirectory.childDirectory('temp')
+        ..createSync(),
+    );
+
+    await expectLater(() async => await artifactUpdater.downloadZipArchive(
+      'test message',
+      Uri.parse('http:///test.zip'),
+      fileSystem.currentDirectory.childDirectory('out'),
+    ), throwsA(isA<ArgumentError>()));
+
+    expect(client.attempts, 1);
+    expect(logger.statusText, contains('test message'));
+    expect(fileSystem.file('out/test'), isNot(exists));
+  });
+
   testWithoutContext('ArtifactUpdater will de-download a file if unzipping fails', () async {
-    final FakeNet net = FakeNet();
     final MockOperatingSystemUtils operatingSystemUtils = MockOperatingSystemUtils();
     final MemoryFileSystem fileSystem = MemoryFileSystem.test();
     final BufferLogger logger = BufferLogger.test();
     final ArtifactUpdater artifactUpdater = ArtifactUpdater(
       fileSystem: fileSystem,
       logger: logger,
-      net: net,
       operatingSystemUtils: operatingSystemUtils,
+      platform: testPlatform,
+      httpClient: MockHttpClient(),
       tempStorage: fileSystem.currentDirectory.childDirectory('temp')
         ..createSync(),
     );
@@ -61,19 +148,18 @@ void main() {
     );
     expect(logger.statusText, contains('test message'));
     expect(fileSystem.file('out/test'), exists);
-    expect(net.attempts, 2);
   });
 
   testWithoutContext('ArtifactUpdater will bail if unzipping fails more than twice', () async {
-    final FakeNet net = FakeNet();
     final MockOperatingSystemUtils operatingSystemUtils = MockOperatingSystemUtils();
     final MemoryFileSystem fileSystem = MemoryFileSystem.test();
     final BufferLogger logger = BufferLogger.test();
     final ArtifactUpdater artifactUpdater = ArtifactUpdater(
       fileSystem: fileSystem,
       logger: logger,
-      net: net,
       operatingSystemUtils: operatingSystemUtils,
+      platform: testPlatform,
+      httpClient: MockHttpClient(),
       tempStorage: fileSystem.currentDirectory.childDirectory('temp')
         ..createSync(),
     );
@@ -89,15 +175,15 @@ void main() {
   });
 
   testWithoutContext('ArtifactUpdater can download a tar archive', () async {
-    final FakeNet net = FakeNet();
     final MockOperatingSystemUtils operatingSystemUtils = MockOperatingSystemUtils();
     final MemoryFileSystem fileSystem = MemoryFileSystem.test();
     final BufferLogger logger = BufferLogger.test();
     final ArtifactUpdater artifactUpdater = ArtifactUpdater(
       fileSystem: fileSystem,
       logger: logger,
-      net: net,
       operatingSystemUtils: operatingSystemUtils,
+      platform: testPlatform,
+      httpClient: MockHttpClient(),
       tempStorage: fileSystem.currentDirectory.childDirectory('temp')
         ..createSync(),
     );
@@ -111,15 +197,15 @@ void main() {
   });
 
   testWithoutContext('ArtifactUpdater will delete downloaded files if they exist.', () async {
-    final FakeNet net = FakeNet();
     final MockOperatingSystemUtils operatingSystemUtils = MockOperatingSystemUtils();
     final MemoryFileSystem fileSystem = MemoryFileSystem.test();
     final BufferLogger logger = BufferLogger.test();
     final ArtifactUpdater artifactUpdater = ArtifactUpdater(
       fileSystem: fileSystem,
       logger: logger,
-      net: net,
       operatingSystemUtils: operatingSystemUtils,
+      platform: testPlatform,
+      httpClient: MockHttpClient(),
       tempStorage: fileSystem.currentDirectory.childDirectory('temp')
         ..createSync(),
     );
@@ -136,25 +222,6 @@ void main() {
   });
 }
 
-class FakeNet implements Net {
-  int attempts = 0;
-
-  @override
-  Future<bool> doesRemoteFileExist(Uri url) async {
-    return true;
-  }
-
-  @override
-  Future<List<int>> fetchUrl(Uri url, {int maxAttempts, File destFile}) async {
-    attempts += 1;
-    if (destFile != null) {
-      destFile.createSync();
-      return null;
-    }
-    return <int>[];
-  }
-}
-
 class MockOperatingSystemUtils extends Mock implements OperatingSystemUtils {
   int failures = 0;
 
@@ -178,3 +245,35 @@ class MockOperatingSystemUtils extends Mock implements OperatingSystemUtils {
       .createSync();
   }
 }
+
+class MockHttpClient extends Mock implements HttpClient {
+  int attempts = 0;
+  bool argumentError = false;
+  final MockHttpClientRequest testRequest = MockHttpClientRequest();
+
+  @override
+  Future<HttpClientRequest> getUrl(Uri url) async {
+    attempts += 1;
+    if (argumentError) {
+      throw ArgumentError();
+    }
+    return testRequest;
+  }
+}
+class MockHttpClientRequest extends Mock implements HttpClientRequest {
+  final MockHttpClientResponse testResponse = MockHttpClientResponse();
+
+  @override
+  Future<HttpClientResponse> close() async {
+    return testResponse;
+  }
+}
+class MockHttpClientResponse extends Mock implements HttpClientResponse {
+  @override
+  int statusCode = HttpStatus.ok;
+
+  @override
+  Future<void> forEach(void Function(List<int> element) action) async {
+    return;
+  }
+}