Fixes a bug where requests made in different Clients would fail (#827)

brianquinlan · web-flow · commit 047d6ed015d3 · 2022-11-22T09:50:14.000-08:00
diff --git a/pkgs/cronet_http/example/integration_test/client_conformance_test.dart b/pkgs/cronet_http/example/integration_test/client_conformance_test.dart
@@ -17,6 +17,8 @@ void main() {
   testRequestHeaders(client);
   testResponseHeaders(client);
   testRedirect(client);
+  testCompressedResponseBody(client);
+  testMultipleClients(CronetClient.new);
 
   // TODO: Use `testAll` when `testServerErrors` passes i.e.
   // testAll(CronetClient(), canStreamRequestBody: false);
diff --git a/pkgs/cupertino_http/CHANGELOG.md b/pkgs/cupertino_http/CHANGELOG.md
@@ -1,3 +1,7 @@
+## 0.0.10
+
+* Fix [Use of multiple CupertinoClients can result in cancelled requests](https://github.com/dart-lang/http/issues/826)
+
 ## 0.0.9
 
 * Add a more complete implementation for `URLSessionTask`:
diff --git a/pkgs/cupertino_http/example/integration_test/client_conformance_test.dart b/pkgs/cupertino_http/example/integration_test/client_conformance_test.dart
@@ -12,12 +12,12 @@ void main() {
   IntegrationTestWidgetsFlutterBinding.ensureInitialized();
 
   group('defaultSessionConfiguration', () {
-    testAll(CupertinoClient.defaultSessionConfiguration(),
+    testAll(CupertinoClient.defaultSessionConfiguration,
         canStreamRequestBody: false);
   });
   group('fromSessionConfiguration', () {
     final config = URLSessionConfiguration.ephemeralSessionConfiguration();
-    testAll(CupertinoClient.fromSessionConfiguration(config),
+    testAll(() => CupertinoClient.fromSessionConfiguration(config),
         canStreamRequestBody: false);
   });
 }
diff --git a/pkgs/cupertino_http/lib/cupertino_client.dart b/pkgs/cupertino_http/lib/cupertino_client.dart
@@ -50,7 +50,7 @@ class _TaskTracker {
 /// }
 /// ```
 class CupertinoClient extends BaseClient {
-  static final Map<int, _TaskTracker> _tasks = {};
+  static final Map<URLSessionTask, _TaskTracker> _tasks = {};
 
   URLSession _urlSession;
 
@@ -143,8 +143,7 @@ class CupertinoClient extends BaseClient {
     }
   }
 
-  static _TaskTracker _tracker(URLSessionTask task) =>
-      _tasks[task.taskIdentifier]!;
+  static _TaskTracker _tracker(URLSessionTask task) => _tasks[task]!;
 
   static void _onComplete(
       URLSession session, URLSessionTask task, Error? error) {
@@ -163,7 +162,7 @@ class CupertinoClient extends BaseClient {
           StateError('task completed without an error or response'));
     }
     taskTracker.close();
-    _tasks.remove(task.taskIdentifier);
+    _tasks.remove(task);
   }
 
   static void _onData(URLSession session, URLSessionTask task, Data data) {
@@ -234,7 +233,7 @@ class CupertinoClient extends BaseClient {
 
     final task = _urlSession.dataTaskWithRequest(urlRequest);
     final taskTracker = _TaskTracker(request);
-    _tasks[task.taskIdentifier] = taskTracker;
+    _tasks[task] = taskTracker;
     task.resume();
 
     final maxRedirects = request.followRedirects ? request.maxRedirects : 0;
diff --git a/pkgs/cupertino_http/lib/cupertino_http.dart b/pkgs/cupertino_http/lib/cupertino_http.dart
@@ -39,6 +39,17 @@ abstract class _ObjectHolder<T extends ncb.NSObject> {
   final T _nsObject;
 
   _ObjectHolder(this._nsObject);
+
+  @override
+  bool operator ==(Object other) {
+    if (other is _ObjectHolder) {
+      return _nsObject == other._nsObject;
+    }
+    return false;
+  }
+
+  @override
+  int get hashCode => _nsObject.hashCode;
 }
 
 /// Settings for controlling whether cookies will be accepted.
diff --git a/pkgs/cupertino_http/pubspec.yaml b/pkgs/cupertino_http/pubspec.yaml
@@ -2,7 +2,7 @@ name: cupertino_http
 description: >
   A macOS/iOS Flutter plugin that provides access to the Foundation URL
   Loading System.
-version: 0.0.9
+version: 0.0.10
 repository: https://github.com/dart-lang/http/tree/master/pkgs/cupertino_http
 
 environment:
diff --git a/pkgs/http/test/html/client_conformance_test.dart b/pkgs/http/test/html/client_conformance_test.dart
@@ -9,9 +9,7 @@ import 'package:http_client_conformance_tests/http_client_conformance_tests.dart
 import 'package:test/test.dart';
 
 void main() {
-  final client = BrowserClient();
-
-  testAll(client,
+  testAll(() => BrowserClient(),
       redirectAlwaysAllowed: true,
       canStreamRequestBody: false,
       canStreamResponseBody: false);
diff --git a/pkgs/http/test/io/client_conformance_test.dart b/pkgs/http/test/io/client_conformance_test.dart
@@ -9,7 +9,5 @@ import 'package:http_client_conformance_tests/http_client_conformance_tests.dart
 import 'package:test/test.dart';
 
 void main() {
-  final client = IOClient();
-
-  testAll(client);
+  testAll(() => IOClient());
 }
diff --git a/pkgs/http_client_conformance_tests/example/client_test.dart b/pkgs/http_client_conformance_tests/example/client_test.dart
@@ -12,6 +12,6 @@ class MyHttpClient extends BaseClient {
 
 void main() {
   group('client conformance tests', () {
-    testAll(MyHttpClient());
+    testAll(() => MyHttpClient());
   });
 }
diff --git a/pkgs/http_client_conformance_tests/lib/http_client_conformance_tests.dart b/pkgs/http_client_conformance_tests/lib/http_client_conformance_tests.dart
@@ -5,6 +5,7 @@
 import 'package:http/http.dart';
 
 import 'src/compressed_response_body_tests.dart';
+import 'src/multiple_clients_tests.dart';
 import 'src/redirect_tests.dart';
 import 'src/request_body_streamed_tests.dart';
 import 'src/request_body_tests.dart';
@@ -16,6 +17,7 @@ import 'src/server_errors_test.dart';
 
 export 'src/compressed_response_body_tests.dart'
     show testCompressedResponseBody;
+export 'src/multiple_clients_tests.dart' show testMultipleClients;
 export 'src/redirect_tests.dart' show testRedirect;
 export 'src/request_body_streamed_tests.dart' show testRequestBodyStreamed;
 export 'src/request_body_tests.dart' show testRequestBody;
@@ -41,18 +43,21 @@ export 'src/server_errors_test.dart' show testServerErrors;
 /// The tests are run against a series of HTTP servers that are started by the
 /// tests. If the tests are run in the browser, then the test servers are
 /// started in another process. Otherwise, the test servers are run in-process.
-void testAll(Client client,
+void testAll(Client Function() clientFactory,
     {bool canStreamRequestBody = true,
     bool canStreamResponseBody = true,
     bool redirectAlwaysAllowed = false}) {
-  testRequestBody(client);
-  testRequestBodyStreamed(client, canStreamRequestBody: canStreamRequestBody);
-  testResponseBody(client, canStreamResponseBody: canStreamResponseBody);
-  testResponseBodyStreamed(client,
+  testRequestBody(clientFactory());
+  testRequestBodyStreamed(clientFactory(),
+      canStreamRequestBody: canStreamRequestBody);
+  testResponseBody(clientFactory(),
       canStreamResponseBody: canStreamResponseBody);
-  testRequestHeaders(client);
-  testResponseHeaders(client);
-  testRedirect(client, redirectAlwaysAllowed: redirectAlwaysAllowed);
-  testServerErrors(client);
-  testCompressedResponseBody(client);
+  testResponseBodyStreamed(clientFactory(),
+      canStreamResponseBody: canStreamResponseBody);
+  testRequestHeaders(clientFactory());
+  testResponseHeaders(clientFactory());
+  testRedirect(clientFactory(), redirectAlwaysAllowed: redirectAlwaysAllowed);
+  testServerErrors(clientFactory());
+  testCompressedResponseBody(clientFactory());
+  testMultipleClients(clientFactory);
 }
diff --git a/pkgs/http_client_conformance_tests/lib/src/multiple_clients_server.dart b/pkgs/http_client_conformance_tests/lib/src/multiple_clients_server.dart
@@ -0,0 +1,35 @@
+// Copyright (c) 2022, 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:async';
+import 'dart:io';
+
+import 'package:stream_channel/stream_channel.dart';
+
+/// Starts an HTTP server that responds the client request path.
+///
+/// Channel protocol:
+///    On Startup:
+///     - send port
+///    When Receive Anything:
+///     - exit
+void hybridMain(StreamChannel<Object?> channel) async {
+  final server = (await HttpServer.bind('localhost', 0))
+    ..listen((request) async {
+      request.response.headers.set('Access-Control-Allow-Origin', '*');
+
+      await request.drain<void>();
+
+      if (request.requestedUri.pathSegments.isNotEmpty) {
+        request.response.write(request.requestedUri.pathSegments.last);
+      }
+      await Future<void>.delayed(const Duration(seconds: 1));
+      await request.response.close();
+    });
+
+  channel.sink.add(server.port);
+  await channel
+      .stream.first; // Any writes indicates that the server should exit.
+  unawaited(server.close());
+}
diff --git a/pkgs/http_client_conformance_tests/lib/src/multiple_clients_server_vm.dart b/pkgs/http_client_conformance_tests/lib/src/multiple_clients_server_vm.dart
diff --git a/pkgs/http_client_conformance_tests/lib/src/multiple_clients_server_web.dart b/pkgs/http_client_conformance_tests/lib/src/multiple_clients_server_web.dart
diff --git a/pkgs/http_client_conformance_tests/lib/src/multiple_clients_tests.dart b/pkgs/http_client_conformance_tests/lib/src/multiple_clients_tests.dart
@@ -0,0 +1,41 @@
+// Copyright (c) 2022, 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 'package:async/async.dart';
+import 'package:http/http.dart';
+import 'package:stream_channel/stream_channel.dart';
+import 'package:test/test.dart';
+
+import 'multiple_clients_server_vm.dart'
+    if (dart.library.html) 'multiple_clients_server_web.dart';
+
+/// Tests that the [Client] works correctly if there are many used
+/// simultaneously.
+void testMultipleClients(Client Function() clientFactory) async {
+  group('test multiple clients', () {
+    late final String host;
+    late final StreamChannel<Object?> httpServerChannel;
+    late final StreamQueue<Object?> httpServerQueue;
+
+    setUpAll(() async {
+      httpServerChannel = await startServer();
+      httpServerQueue = StreamQueue(httpServerChannel.stream);
+      host = 'localhost:${await httpServerQueue.next}';
+    });
+    tearDownAll(() => httpServerChannel.sink.add(null));
+
+    test('multiple clients with simultaneous requests', () async {
+      final responseFutures = <Future<Response>>[];
+      for (var i = 0; i < 5; ++i) {
+        final client = clientFactory();
+        responseFutures.add(client.get(Uri.http(host, '/$i')));
+      }
+      final responses = await Future.wait(responseFutures);
+      for (var i = 0; i < 5; ++i) {
+        expect(responses[i].statusCode, 200);
+        expect(responses[i].body, i.toString());
+      }
+    });
+  });
+}

Original file line number	Diff line number	Diff line change
`@@ -9,7 +9,5 @@ import 'package:http_client_conformance_tests/http_client_conformance_tests.dart`
`9`	`9`	`import 'package:test/test.dart';`
`10`	`10`
`11`	`11`	`void main() {`
`12`		`- final client = IOClient();`
`13`		`-`
`14`		`- testAll(client);`
	`12`	`+ testAll(() => IOClient());`
`15`	`13`	`}`
Original file line number	Diff line number	Diff line change
`@@ -12,6 +12,6 @@ class MyHttpClient extends BaseClient {`
`12`	`12`
`13`	`13`	`void main() {`
`14`	`14`	`group('client conformance tests', () {`
`15`		`- testAll(MyHttpClient());`
	`15`	`+ testAll(() => MyHttpClient());`
`16`	`16`	`});`
`17`	`17`	`}`