[dart:io] Fix HttpClient close() calling itself.

Call close() on HttpClient will close all connections. Whenever a
connection is closed, it will notify HttpClient to close() again, which
is unnecessary. When there are a large amount of servers need to be
closed, it is likely overflow the stack.

Bug: https://github.com/dart-lang/sdk/issues/41247
Change-Id: I62afb1a60d3e4581aa102628e76f717c28031c2f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/141844
Commit-Queue: Zichang Guo <zichangguo@google.com>
Reviewed-by: Lasse R.H. Nielsen <lrn@google.com>
Reviewed-by: Siva Annamalai <asiva@google.com>

CHANGELOG.md

@@ -6,7 +6,7 @@
 
 *   Adds `Abort` method to class `HttpClientRequest`, which allows users
     to cancel outgoing HTTP requests and stop following IO operations.
-*   A validtion check is added to `path` of class `Cookie`. Having characters
+*   A validation check is added to `path` of class `Cookie`. Having characters
     ranging from 0x00 to 0x1f and 0x3b (";") will lead to a `FormatException`.
 
 #### `dart:typed_data`

sdk/lib/_http/http_impl.dart

@@ -1988,6 +1988,12 @@ class _HttpClientConnection {
     _socket.destroy();
   }
 
+  void destroyFromExternal() {
+    closed = true;
+    _httpClient._connectionClosedNoFurtherClosing(this);
+    _socket.destroy();
+  }
+
   void close() {
     closed = true;
     _httpClient._connectionClosed(this);
@@ -1996,6 +2002,14 @@ class _HttpClientConnection {
         .then((_) => _socket.destroy());
   }
 
+  void closeFromExternal() {
+    closed = true;
+    _httpClient._connectionClosedNoFurtherClosing(this);
+    _streamFuture!
+        .timeout(_httpClient.idleTimeout)
+        .then((_) => _socket.destroy());
+  }
+
   Future<_HttpClientConnection> createProxyTunnel(
       String host,
       int port,
@@ -2141,14 +2155,14 @@ class _ConnectionTarget {
     }
     if (force) {
       for (var c in _idle.toList()) {
-        c.destroy();
+        c.destroyFromExternal();
       }
       for (var c in _active.toList()) {
-        c.destroy();
+        c.destroyFromExternal();
       }
     } else {
       for (var c in _idle.toList()) {
-        c.close();
+        c.closeFromExternal();
       }
     }
   }
@@ -2486,6 +2500,20 @@ class _HttpClient implements HttpClient {
     }
   }
 
+  // Remove a closed connection and not issue _closeConnections(). If the close
+  // is signaled from user by calling close(), _closeConnections() was called
+  // and prevent further calls.
+  void _connectionClosedNoFurtherClosing(_HttpClientConnection connection) {
+    connection.stopTimer();
+    var connectionTarget = _connectionTargets[connection.key];
+    if (connectionTarget != null) {
+      connectionTarget.connectionClosed(connection);
+      if (connectionTarget.isEmpty) {
+        _connectionTargets.remove(connection.key);
+      }
+    }
+  }
+
   void _connectionsChanged() {
     if (_closing) {
       _closeConnections(_closingForcefully);

tests/standalone/io/http_close_stack_overflow_test.dart

@@ -0,0 +1,37 @@
+// Copyright (c) 2020, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'dart:io';
+
+// Test that closing a large amount of servers will not lead to a stack
+// overflow.
+Future<void> main() async {
+  final max = 10000;
+  final servers = <ServerSocket>[];
+  for (var i = 0; i < max; i++) {
+    final server = await ServerSocket.bind("localhost", 0);
+    server.listen((Socket socket) {});
+    servers.add(server);
+  }
+  final client = HttpClient();
+  var got = 0;
+  for (var i = 0; i < max; i++) {
+    new Future(() async {
+      try {
+        final request = await client
+            .getUrl(Uri.parse("http://localhost:${servers[i].port}/"));
+        got++;
+        if (got == max) {
+          // Test that no stack overflow happens.
+          client.close(force: true);
+          for (final server in servers) {
+            server.close();
+          }
+        }
+        final response = await request.close();
+        response.drain();
+      } on HttpException catch (_) {}
+    });
+  }
+}

tests/standalone/standalone.status

@@ -5,6 +5,7 @@
 # Tests using the multitest feature where failure is expected should *also* be
 # listed in tests/lib/analyzer/analyze_tests.status without the "standalone"
 # prefix.
+io/http_close_stack_overflow_test: Skip # The test is heavy loaded. Should be used for manual test.
 io/http_linklocal_ipv6_test: SkipByDesign # This needs manual test.
 io/non_utf8_directory_test: Skip # Issue 33519. Temp files causing bots to go purple.
 io/non_utf8_file_test: Skip # Issue 33519. Temp files causing bots to go purple.

tests/standalone_2/io/http_close_stack_overflow_test.dart

@@ -0,0 +1,37 @@
+// Copyright (c) 2020, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'dart:io';
+
+// Test that closing a large amount of servers will not lead to a stack
+// overflow.
+Future<void> main() async {
+  final max = 10000;
+  final servers = <ServerSocket>[];
+  for (var i = 0; i < max; i++) {
+    final server = await ServerSocket.bind("localhost", 0);
+    server.listen((Socket socket) {});
+    servers.add(server);
+  }
+  final client = HttpClient();
+  var got = 0;
+  for (var i = 0; i < max; i++) {
+    new Future(() async {
+      try {
+        final request = await client
+            .getUrl(Uri.parse("http://localhost:${servers[i].port}/"));
+        got++;
+        if (got == max) {
+          // Test that no stack overflow happens.
+          client.close(force: true);
+          for (final server in servers) {
+            server.close();
+          }
+        }
+        final response = await request.close();
+        response.drain();
+      } on HttpException catch (_) {}
+    });
+  }
+}

tests/standalone_2/standalone_2.status

@@ -5,6 +5,7 @@
 # Tests using the multitest feature where failure is expected should *also* be
 # listed in tests/lib/analyzer/analyze_tests.status without the "standalone"
 # prefix.
+io/http_close_stack_overflow_test: Skip # The test is heavy loaded. Should be used for manual test.
 io/http_linklocal_ipv6_test: SkipByDesign # This needs manual test.
 io/non_utf8_directory_test: Skip # Issue 33519. Temp files causing bots to go purple.
 io/non_utf8_file_test: Skip # Issue 33519. Temp files causing bots to go purple.