From 18cfc0a17f3144e6eb4a72a14bb9fd61d20a216e Mon Sep 17 00:00:00 2001 From: Zichang Guo Date: Fri, 28 Aug 2020 21:58:08 +0000 Subject: [PATCH] [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 Reviewed-by: Lasse R.H. Nielsen Reviewed-by: Siva Annamalai --- CHANGELOG.md | 2 +- sdk/lib/_http/http_impl.dart | 34 +++++++++++++++-- .../io/http_close_stack_overflow_test.dart | 37 +++++++++++++++++++ tests/standalone/standalone.status | 1 + .../io/http_close_stack_overflow_test.dart | 37 +++++++++++++++++++ tests/standalone_2/standalone_2.status | 1 + 6 files changed, 108 insertions(+), 4 deletions(-) create mode 100644 tests/standalone/io/http_close_stack_overflow_test.dart create mode 100644 tests/standalone_2/io/http_close_stack_overflow_test.dart diff --git a/CHANGELOG.md b/CHANGELOG.md index f6053207c3f..9e6332c5e96 100644 --- a/CHANGELOG.md +++ b/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` diff --git a/sdk/lib/_http/http_impl.dart b/sdk/lib/_http/http_impl.dart index f01e495df54..3cf75d9f43a 100644 --- a/sdk/lib/_http/http_impl.dart +++ b/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); diff --git a/tests/standalone/io/http_close_stack_overflow_test.dart b/tests/standalone/io/http_close_stack_overflow_test.dart new file mode 100644 index 00000000000..27055f8467b --- /dev/null +++ b/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 main() async { + final max = 10000; + final servers = []; + 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 (_) {} + }); + } +} diff --git a/tests/standalone/standalone.status b/tests/standalone/standalone.status index fd4c0887536..73137e9b1c6 100644 --- a/tests/standalone/standalone.status +++ b/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. diff --git a/tests/standalone_2/io/http_close_stack_overflow_test.dart b/tests/standalone_2/io/http_close_stack_overflow_test.dart new file mode 100644 index 00000000000..27055f8467b --- /dev/null +++ b/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 main() async { + final max = 10000; + final servers = []; + 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 (_) {} + }); + } +} diff --git a/tests/standalone_2/standalone_2.status b/tests/standalone_2/standalone_2.status index f99d678bedd..257680bfbd5 100644 --- a/tests/standalone_2/standalone_2.status +++ b/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.