Added timeout for closing devfs sync http connections. (flutter#66152)

gaaclarke · web-flow · commit 76ad864727d3 · 2020-09-25T10:52:33.000-07:00
diff --git a/packages/flutter_tools/lib/src/devfs.dart b/packages/flutter_tools/lib/src/devfs.dart
@@ -239,7 +239,11 @@ class _DevFSHttpWriter implements DevFSWriter {
   final String fsName;
   final Uri httpAddress;
 
-  static const int kMaxInFlight = 6;
+  // 3 was chosen to try to limit the varience in the time it takes to execute
+  // `await request.close()` since there is a known bug in Dart where it doesn't
+  // always return a status code in response to a PUT request:
+  // https://github.com/dart-lang/sdk/issues/43525.
+  static const int kMaxInFlight = 3;
 
   int _inFlight = 0;
   Map<Uri, DevFSContent> _outstanding;
@@ -289,13 +293,29 @@ class _DevFSHttpWriter implements DevFSWriter {
           _osUtils,
         );
         await request.addStream(contents);
-        final HttpClientResponse response = await request.close();
-        response.listen((_) {},
-          onError: (dynamic error) {
-            _logger.printTrace('error: $error');
-          },
-          cancelOnError: true,
-        );
+        // The contents has already been streamed, closing the request should
+        // not take long but we are experiencing hangs with it, see #63869.
+        //
+        // Once the bug in Dart is solved we can remove the timeout
+        // (https://github.com/dart-lang/sdk/issues/43525).  The timeout was
+        // chosen to be inflated based on the max observed time when running the
+        // tests in "Google Tests".
+        try {
+          final HttpClientResponse response = await request.close().timeout(
+            const Duration(milliseconds: 10000));
+          response.listen((_) {},
+            onError: (dynamic error) {
+              _logger.printTrace('error: $error');
+            },
+            cancelOnError: true,
+          );
+        } on TimeoutException {
+          request.abort();
+          // This should throw "HttpException: Request has been aborted".
+          await request.done;
+          // Just to be safe we rethrow the TimeoutException.
+          rethrow;
+        }
         break;
       } on Exception catch (error, trace) {
         if (!_completer.isCompleted) {
diff --git a/packages/flutter_tools/test/general.shard/devfs_test.dart b/packages/flutter_tools/test/general.shard/devfs_test.dart
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import 'dart:async';
 import 'dart:convert';
 
 import 'package:file/file.dart';
@@ -258,7 +259,7 @@ void main() {
     expect(devFS.lastCompiled, isNot(previousCompile));
   });
 
-   testWithoutContext('DevFS uses provided DevFSWriter instead of default HTTP writer', () async {
+  testWithoutContext('DevFS uses provided DevFSWriter instead of default HTTP writer', () async {
     final FileSystem fileSystem = MemoryFileSystem.test();
     final FakeDevFSWriter writer = FakeDevFSWriter();
     final FakeVmServiceHost fakeVmServiceHost = FakeVmServiceHost(
@@ -332,6 +333,84 @@ void main() {
       Uri.parse('goodbye'): DevFSFileContent(file),
     }, Uri.parse('/foo/bar/devfs/')), throwsA(isA<DevFSException>()));
   });
+
+  testWithoutContext('test handles request closure hangs', () async {
+    final FileSystem fileSystem = MemoryFileSystem.test();
+    final FakeVmServiceHost fakeVmServiceHost = FakeVmServiceHost(
+      requests: <VmServiceExpectation>[createDevFSRequest],
+    );
+    final HttpClient httpClient = MockHttpClient();
+    final MockHttpClientRequest httpRequest = MockHttpClientRequest();
+    when(httpRequest.headers).thenReturn(MockHttpHeaders());
+    when(httpClient.putUrl(any)).thenAnswer((Invocation invocation) {
+      return Future<HttpClientRequest>.value(httpRequest);
+    });
+    int closeCount = 0;
+    final Completer<MockHttpClientResponse> hanger = Completer<MockHttpClientResponse>();
+    final Completer<MockHttpClientResponse> succeeder = Completer<MockHttpClientResponse>();
+    final List<Completer<MockHttpClientResponse>> closeCompleters =
+      <Completer<MockHttpClientResponse>>[hanger, succeeder];
+    succeeder.complete(MockHttpClientResponse());
+
+    when(httpRequest.close()).thenAnswer((Invocation invocation) {
+      final Completer<MockHttpClientResponse> completer = closeCompleters[closeCount];
+      closeCount += 1;
+      return completer.future;
+    });
+    when(httpRequest.abort()).thenAnswer((_) {
+      hanger.completeError(const HttpException('aborted'));
+    });
+    when(httpRequest.done).thenAnswer((_) {
+      if (closeCount == 1) {
+        return hanger.future;
+      } else if (closeCount == 2) {
+        return succeeder.future;
+      } else {
+        // This branch shouldn't happen.
+        fail('This branch should not happen');
+      }
+    });
+
+    final BufferLogger logger = BufferLogger.test();
+    final DevFS devFS = DevFS(
+      fakeVmServiceHost.vmService,
+      'test',
+      fileSystem.currentDirectory,
+      fileSystem: fileSystem,
+      logger: logger,
+      osUtils: FakeOperatingSystemUtils(),
+      httpClient: httpClient,
+    );
+
+    await devFS.create();
+    final DateTime previousCompile = devFS.lastCompiled;
+
+    final MockResidentCompiler residentCompiler = MockResidentCompiler();
+    when(residentCompiler.recompile(
+      any,
+      any,
+      outputPath: anyNamed('outputPath'),
+      packageConfig: anyNamed('packageConfig'),
+    )).thenAnswer((Invocation invocation) async {
+      fileSystem.file('example').createSync();
+      return const CompilerOutput('lib/foo.txt.dill', 0, <Uri>[]);
+    });
+
+    final UpdateFSReport report = await devFS.update(
+      mainUri: Uri.parse('lib/main.dart'),
+      generator: residentCompiler,
+      dillOutputPath: 'lib/foo.dill',
+      pathToReload: 'lib/foo.txt.dill',
+      trackWidgetCreation: false,
+      invalidatedFiles: <Uri>[],
+      packageConfig: PackageConfig.empty,
+    );
+
+    expect(report.success, true);
+    expect(devFS.lastCompiled, isNot(previousCompile));
+    expect(closeCount, 2);
+    expect(logger.errorText, '');
+  });
 }
 
 class MockHttpClientRequest extends Mock implements HttpClientRequest {}
