From 1b8382b14d5cf24d6d5ebe3a2d74fba286c45a12 Mon Sep 17 00:00:00 2001 From: Brian Quinlan Date: Tue, 14 Nov 2023 16:32:50 +0000 Subject: [PATCH] Revert "Call the HttpClient constructor when connecting a WebSocket to allow HttpOverrides to work." This reverts commit 92e7989fbcf0296231df34925f4333d124b30138. Reason for revert: Causes VmService WebSocket creating to fail because HttpClient is mocked during Flutter Widget tests. See https://github.com/flutter/flutter/issues/138413 Original change's description: > Call the HttpClient constructor when connecting a WebSocket to allow HttpOverrides to work. > > One consequence of this is that there will be one HttpClient per WebSocket connection rather than one for all WebSocket connections. This does *not* affect connection pooling because the sockets are detached from the HttpClient after the WebSocket connection is made. > > Closes https://github.com/dart-lang/sdk/pull/52509 > > GitOrigin-RevId: 28ca642a56e3bbc6112df844b07026b9e0ae8ae6 > Change-Id: I8d9c6f2a883b0f79bafca30abd1dfd98291cf0e0 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/305620 > Reviewed-by: Alexander Aprelev > Commit-Queue: Brian Quinlan Change-Id: Ie1d029e7acf477503cd601f6a03e121cd17d1695 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/336100 Auto-Submit: Brian Quinlan Bot-Commit: Rubber Stamper Commit-Queue: Rubber Stamper Commit-Queue: Brian Quinlan Reviewed-by: Alexander Aprelev --- sdk/lib/_http/websocket.dart | 12 +- sdk/lib/_http/websocket_impl.dart | 17 +-- .../io/http_override_websocket_test.dart | 82 -------------- tests/standalone/io/web_socket_test.dart | 103 ++++++------------ 4 files changed, 38 insertions(+), 176 deletions(-) delete mode 100644 tests/standalone/io/http_override_websocket_test.dart diff --git a/sdk/lib/_http/websocket.dart b/sdk/lib/_http/websocket.dart index a4791ab7053..4ef5b08e13a 100644 --- a/sdk/lib/_http/websocket.dart +++ b/sdk/lib/_http/websocket.dart @@ -311,9 +311,6 @@ abstract class WebSocket /// /// If the `url` contains user information this will be passed as basic /// authentication when setting up the connection. - /// - /// If [customClient] is provided, then it will be used to make the - /// WebSocket connection and the [userAgent] field is ignored. static Future connect(String url, {Iterable? protocols, Map? headers, @@ -399,15 +396,10 @@ abstract class WebSocket /// not, the receiving end will close the connection. void addUtf8Text(List bytes); - /// The `User-Agent` header used for WebSocket connections. - /// - /// If the userAgent is set to `null`, no "User-Agent" header will be added - /// to requests. - /// - /// If [connect] is called with a `customClient`, then the WebSocket will - /// not use [userAgent]. + /// Gets the user agent used for WebSocket connections. static String? get userAgent => _WebSocketImpl.userAgent; + /// Sets the user agent to use for WebSocket connections. static set userAgent(String? userAgent) { _WebSocketImpl.userAgent = userAgent; } diff --git a/sdk/lib/_http/websocket_impl.dart b/sdk/lib/_http/websocket_impl.dart index f527eee11d5..b900c182a39 100644 --- a/sdk/lib/_http/websocket_impl.dart +++ b/sdk/lib/_http/websocket_impl.dart @@ -989,7 +989,7 @@ class _WebSocketImpl extends Stream with _ServiceObject implements WebSocket { Timer? _closeTimer; _WebSocketPerMessageDeflate? _deflate; - static String? _userAgent = _getHttpVersion(); + static final HttpClient _httpClient = HttpClient(); static Future connect( String url, Iterable? protocols, Map? headers, @@ -1018,16 +1018,7 @@ class _WebSocketImpl extends Stream with _ServiceObject implements WebSocket { path: uri.path, query: uri.query, fragment: uri.fragment); - - // The default userAgent is only overridden if no custom HttpClient is set. - late HttpClient client; - if (customClient != null) { - client = customClient; - } else { - client = HttpClient(); - client.userAgent = _userAgent; - } - return client.openUrl("GET", uri).then((request) { + return (customClient ?? _httpClient).openUrl("GET", uri).then((request) { if (uri.userInfo != null && uri.userInfo.isNotEmpty) { // If the URL contains user information use that for basic // authorization. @@ -1273,10 +1264,10 @@ class _WebSocketImpl extends Stream with _ServiceObject implements WebSocket { return _sink.close(); } - static String? get userAgent => _userAgent; + static String? get userAgent => _httpClient.userAgent; static set userAgent(String? userAgent) { - _userAgent = userAgent; + _httpClient.userAgent = userAgent; } void _close([int? code, String? reason]) { diff --git a/tests/standalone/io/http_override_websocket_test.dart b/tests/standalone/io/http_override_websocket_test.dart deleted file mode 100644 index 7e296a4db6f..00000000000 --- a/tests/standalone/io/http_override_websocket_test.dart +++ /dev/null @@ -1,82 +0,0 @@ -// Copyright (c) 2023, 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'; - -import "package:async_helper/async_helper.dart"; -import "package:expect/expect.dart"; - -class TestQueryParamAddingHttpClient implements HttpClient { - Map _queryParams; - final _client = HttpClient(); - - TestQueryParamAddingHttpClient(this._queryParams); - - set userAgent(String? agent) => _client.userAgent = agent; - String? get userAgent => _client.userAgent; - - Future openUrl(String method, Uri url) { - return _client.openUrl(method, url.replace(queryParameters: _queryParams)); - } - - void close({bool force = false}) => _client.close(force: force); - - dynamic noSuchMethod(Invocation invocation) { - throw UnsupportedError(invocation.memberName.toString()); - } -} - -Future testZoneHttpClientUsedInWebSocket() async { - final server = await HttpServer.bind("localhost", 0); - String? recordedUserAgent; - Map? recordedQueryParameters; - - server.forEach((request) { - recordedUserAgent = request.headers.value(HttpHeaders.userAgentHeader); - recordedQueryParameters = request.uri.queryParameters; - WebSocketTransformer.upgrade(request) - .then((webSocket) => webSocket.close()); - }); - - WebSocket.userAgent = 'Agent Smith'; - - final client1 = TestQueryParamAddingHttpClient({"test": "1"}); - client1.userAgent = "ZoneAgent1"; - await HttpOverrides.runZoned( - () async { - final webSocket = - await WebSocket.connect("ws://localhost:${server.port}"); - await webSocket.close(); - }, - createHttpClient: (c) => client1, - ); - Expect.equals("Agent Smith", recordedUserAgent); - Expect.mapEquals({"test": "1"}, recordedQueryParameters!); - - // The `HttpClient` used by `WebSocket` used to be static, so it is worth - // testing that the value used in the first execution is not reused. - final client2 = TestQueryParamAddingHttpClient({"test": "2"}); - WebSocket.userAgent = 'Agent X'; - client2.userAgent = "ZoneAgent2"; - await HttpOverrides.runZoned( - () async { - final webSocket = - await WebSocket.connect("ws://localhost:${server.port}"); - await webSocket.close(); - }, - createHttpClient: (c) => client2, - ); - Expect.equals("Agent X", recordedUserAgent); - Expect.mapEquals({"test": "2"}, recordedQueryParameters!); - - client1.close(); - client2.close(); - server.close(); -} - -main() async { - asyncStart(); - await testZoneHttpClientUsedInWebSocket(); - asyncEnd(); -} diff --git a/tests/standalone/io/web_socket_test.dart b/tests/standalone/io/web_socket_test.dart index 6083dd715d7..99a42878444 100644 --- a/tests/standalone/io/web_socket_test.dart +++ b/tests/standalone/io/web_socket_test.dart @@ -542,78 +542,42 @@ class SecurityConfiguration { }); } - Future testUserAgentSetToString() async { + void testShouldSetUserAgent() { asyncStart(); + createServer().then((server) { + server.transform(new WebSocketTransformer()).listen((webSocket) { + Expect.equals('Custom User Agent', WebSocket.userAgent); + server.close(); + webSocket.close(); + asyncEnd(); + }); - final server = await HttpServer.bind("localhost", 0); - String? recordedUserAgent; - server.forEach((request) { - recordedUserAgent = request.headers.value(HttpHeaders.userAgentHeader); - WebSocketTransformer.upgrade(request) - .then((webSocket) => webSocket.close()); + WebSocket.userAgent = 'Custom User Agent'; + createClient(server.port).then((webSocket) { + webSocket.close(); + }); }); - - WebSocket.userAgent = "Agent Smith"; - final webSocket = await WebSocket.connect("ws://localhost:${server.port}"); - - Expect.equals("Agent Smith", recordedUserAgent); - - await webSocket.close(); - await server.close(); - - asyncEnd(); } - Future testUserAgentSetToNull() async { + void testStaticClientUserAgentStaysTheSame() { asyncStart(); - - final server = await HttpServer.bind("localhost", 0); - String? recordedUserAgent; - server.forEach((request) { - recordedUserAgent = request.headers.value(HttpHeaders.userAgentHeader); - WebSocketTransformer.upgrade(request) - .then((webSocket) => webSocket.close()); + createServer().then((server) { + server.transform(new WebSocketTransformer()).listen((webSocket) { + Expect.equals('Custom User Agent', WebSocket.userAgent); + server.close(); + webSocket.close(); + asyncEnd(); + }); + // Next line should take no effect on custom user agent value provided + WebSocket.userAgent = 'Custom User Agent'; + createClient(server.port, customUserAgent: 'New User Agent') + .then((webSocket) { + webSocket.close(); + }); }); - - WebSocket.userAgent = null; - final webSocket = await WebSocket.connect("ws://localhost:${server.port}"); - - Expect.equals(null, recordedUserAgent); - - await webSocket.close(); - await server.close(); - - asyncEnd(); } - Future testUserAgentSetToStringAndCustomClient() async { - asyncStart(); - - final server = await HttpServer.bind("localhost", 0); - String? recordedUserAgent; - server.forEach((request) { - recordedUserAgent = request.headers.value(HttpHeaders.userAgentHeader); - WebSocketTransformer.upgrade(request) - .then((webSocket) => webSocket.close()); - }); - - final client = HttpClient(); - client.userAgent = "Agent Jones"; - WebSocket.userAgent = "Agent X"; - final webSocket = await WebSocket.connect("ws://localhost:${server.port}", - customClient: client); - - Expect.equals("Agent Jones", recordedUserAgent); - Expect.equals("Agent Jones", client.userAgent); - Expect.equals("Agent X", WebSocket.userAgent); - - await webSocket.close(); - await server.close(); - - asyncEnd(); - } - - Future runTests() async { + void runTests() { testRequestResponseClientCloses(2, null, null, 1); testRequestResponseClientCloses(2, 3001, null, 2); testRequestResponseClientCloses(2, 3002, "Got tired", 3); @@ -640,15 +604,12 @@ class SecurityConfiguration { testFromUpgradedSocket(); testAdditionalHeaders(); testBasicAuthentication(); - await testUserAgentSetToString(); - await testUserAgentSetToNull(); - await testUserAgentSetToStringAndCustomClient(); + testShouldSetUserAgent(); + testStaticClientUserAgentStaysTheSame(); } } -main() async { - asyncStart(); - await new SecurityConfiguration(secure: false).runTests(); - await new SecurityConfiguration(secure: true).runTests(); - asyncEnd(); +main() { + new SecurityConfiguration(secure: false).runTests(); + new SecurityConfiguration(secure: true).runTests(); }