From 7586bc5612604ad713613186551c3fe9a5633ed5 Mon Sep 17 00:00:00 2001 From: Brian Quinlan Date: Fri, 29 Mar 2024 10:50:50 -0700 Subject: [PATCH 1/4] Integrate cronet_http with http_profile --- .../integration_test/client_profile_test.dart | 288 ++++++++++++++++++ pkgs/cronet_http/example/pubspec.yaml | 2 + pkgs/cronet_http/lib/src/cronet_client.dart | 89 +++++- pkgs/cronet_http/pubspec.yaml | 3 + 4 files changed, 378 insertions(+), 4 deletions(-) create mode 100644 pkgs/cronet_http/example/integration_test/client_profile_test.dart diff --git a/pkgs/cronet_http/example/integration_test/client_profile_test.dart b/pkgs/cronet_http/example/integration_test/client_profile_test.dart new file mode 100644 index 0000000000..3e17327ce8 --- /dev/null +++ b/pkgs/cronet_http/example/integration_test/client_profile_test.dart @@ -0,0 +1,288 @@ +// Copyright (c) 2024, 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:cronet_http/src/cronet_client.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:http/http.dart'; +import 'package:http_profile/http_profile.dart'; +import 'package:integration_test/integration_test.dart'; + +void main() { + IntegrationTestWidgetsFlutterBinding.ensureInitialized(); + + group('profile', () { + final profilingEnabled = HttpClientRequestProfile.profilingEnabled; + + setUpAll(() { + HttpClientRequestProfile.profilingEnabled = true; + }); + + tearDownAll(() { + HttpClientRequestProfile.profilingEnabled = profilingEnabled; + }); + + group('POST', () { + late HttpServer successServer; + late Uri successServerUri; + late HttpClientRequestProfile profile; + + setUpAll(() async { + successServer = (await HttpServer.bind('localhost', 0)) + ..listen((request) async { + await request.drain(); + request.response.headers.set('Content-Type', 'text/plain'); + request.response.headers.set('Content-Length', '11'); + request.response.write('Hello World'); + await request.response.close(); + }); + successServerUri = Uri.http('localhost:${successServer.port}'); + final client = CronetClientWithProfile.defaultCronetEngine(); + await client.post(successServerUri, + headers: {'Content-Type': 'text/plain'}, body: 'Hi'); + profile = client.profile!; + }); + tearDownAll(() { + successServer.close(); + }); + + test('profile attributes', () { + expect(profile.events, isEmpty); + expect(profile.requestMethod, 'POST'); + expect(profile.requestUri, successServerUri.toString()); + expect(profile.connectionInfo, + containsPair('package', 'package:cronet_http')); + }); + + test('request attributes', () { + expect(profile.requestData.bodyBytes, 'Hi'.codeUnits); + expect(profile.requestData.contentLength, 2); + expect(profile.requestData.endTime, isNotNull); + expect(profile.requestData.error, isNull); + expect( + profile.requestData.headers, containsPair('Content-Length', ['2'])); + expect(profile.requestData.headers, + containsPair('Content-Type', ['text/plain; charset=utf-8'])); + expect(profile.requestData.persistentConnection, isNull); + expect(profile.requestData.proxyDetails, isNull); + expect(profile.requestData.startTime, isNotNull); + }); + + test('response attributes', () { + expect(profile.responseData.bodyBytes, 'Hello World'.codeUnits); + expect(profile.responseData.compressionState, isNull); + expect(profile.responseData.contentLength, 11); + expect(profile.responseData.endTime, isNotNull); + expect(profile.responseData.error, isNull); + expect(profile.responseData.headers, + containsPair('content-type', ['text/plain'])); + expect(profile.responseData.headers, + containsPair('content-length', ['11'])); + expect(profile.responseData.isRedirect, false); + expect(profile.responseData.persistentConnection, isNull); + expect(profile.responseData.reasonPhrase, 'OK'); + expect(profile.responseData.redirects, isEmpty); + expect(profile.responseData.startTime, isNotNull); + expect(profile.responseData.statusCode, 200); + }); + }); + + group('failed POST request', () { + late HttpClientRequestProfile profile; + + setUpAll(() async { + final client = CronetClientWithProfile.defaultCronetEngine(); + try { + await client.post(Uri.http('thisisnotahost'), + headers: {'Content-Type': 'text/plain'}, body: 'Hi'); + fail('expected exception'); + } on ClientException { + // Expected exception. + } + profile = client.profile!; + }); + + test('profile attributes', () { + expect(profile.events, isEmpty); + expect(profile.requestMethod, 'POST'); + expect(profile.requestUri, 'http://thisisnotahost'); + expect(profile.connectionInfo, + containsPair('package', 'package:cronet_http')); + }); + + test('request attributes', () { + expect(profile.requestData.bodyBytes, 'Hi'.codeUnits); + expect(profile.requestData.contentLength, 2); + expect(profile.requestData.endTime, isNotNull); + expect(profile.requestData.error, startsWith('ClientException:')); + expect( + profile.requestData.headers, containsPair('Content-Length', ['2'])); + expect(profile.requestData.headers, + containsPair('Content-Type', ['text/plain; charset=utf-8'])); + expect(profile.requestData.persistentConnection, isNull); + expect(profile.requestData.proxyDetails, isNull); + expect(profile.requestData.startTime, isNotNull); + }); + + test('response attributes', () { + expect(profile.responseData.bodyBytes, isEmpty); + expect(profile.responseData.compressionState, isNull); + expect(profile.responseData.contentLength, isNull); + expect(profile.responseData.endTime, isNull); + expect(profile.responseData.error, isNull); + expect(profile.responseData.headers, isNull); + expect(profile.responseData.isRedirect, isNull); + expect(profile.responseData.persistentConnection, isNull); + expect(profile.responseData.reasonPhrase, isNull); + expect(profile.responseData.redirects, isEmpty); + expect(profile.responseData.startTime, isNull); + expect(profile.responseData.statusCode, isNull); + }); + }); + + group('failed POST response', () { + late HttpServer successServer; + late Uri successServerUri; + late HttpClientRequestProfile profile; + + setUpAll(() async { + successServer = (await HttpServer.bind('localhost', 0)) + ..listen((request) async { + await request.drain(); + request.response.headers.set('Content-Type', 'text/plain'); + request.response.headers.set('Content-Length', '11'); + final socket = await request.response.detachSocket(); + await socket.close(); + }); + successServerUri = Uri.http('localhost:${successServer.port}'); + final client = CronetClientWithProfile.defaultCronetEngine(); + + try { + await client.post(successServerUri, + headers: {'Content-Type': 'text/plain'}, body: 'Hi'); + fail('expected exception'); + } on ClientException { + // Expected exception. + } + profile = client.profile!; + }); + tearDownAll(() { + successServer.close(); + }); + + test('profile attributes', () { + expect(profile.events, isEmpty); + expect(profile.requestMethod, 'POST'); + expect(profile.requestUri, successServerUri.toString()); + expect(profile.connectionInfo, + containsPair('package', 'package:cronet_http')); + }); + + test('request attributes', () { + expect(profile.requestData.bodyBytes, 'Hi'.codeUnits); + expect(profile.requestData.contentLength, 2); + expect(profile.requestData.endTime, isNotNull); + expect(profile.requestData.error, isNull); + expect( + profile.requestData.headers, containsPair('Content-Length', ['2'])); + expect(profile.requestData.headers, + containsPair('Content-Type', ['text/plain; charset=utf-8'])); + expect(profile.requestData.persistentConnection, isNull); + expect(profile.requestData.proxyDetails, isNull); + expect(profile.requestData.startTime, isNotNull); + }); + + test('response attributes', () { + expect(profile.responseData.bodyBytes, isEmpty); + expect(profile.responseData.compressionState, isNull); + expect(profile.responseData.contentLength, 11); + expect(profile.responseData.endTime, isNotNull); + expect(profile.responseData.error, startsWith('ClientException:')); + expect(profile.responseData.headers, + containsPair('content-type', ['text/plain'])); + expect(profile.responseData.headers, + containsPair('content-length', ['11'])); + expect(profile.responseData.isRedirect, false); + expect(profile.responseData.persistentConnection, isNull); + expect(profile.responseData.reasonPhrase, 'OK'); + expect(profile.responseData.redirects, isEmpty); + expect(profile.responseData.startTime, isNotNull); + expect(profile.responseData.statusCode, 200); + }); + }); + + group('redirects', () { + late HttpServer successServer; + late Uri successServerUri; + late HttpClientRequestProfile profile; + + setUpAll(() async { + successServer = (await HttpServer.bind('localhost', 0)) + ..listen((request) async { + if (request.requestedUri.pathSegments.isEmpty) { + unawaited(request.response.close()); + } else { + final n = int.parse(request.requestedUri.pathSegments.last); + final nextPath = n - 1 == 0 ? '' : '${n - 1}'; + unawaited(request.response + .redirect(successServerUri.replace(path: '/$nextPath'))); + } + }); + successServerUri = Uri.http('localhost:${successServer.port}'); + }); + tearDownAll(() { + successServer.close(); + }); + + test('no redirects', () async { + final client = CronetClientWithProfile.defaultCronetEngine(); + await client.get(successServerUri); + profile = client.profile!; + + expect(profile.responseData.redirects, isEmpty); + }); + + test('follow redirects', () async { + final client = CronetClientWithProfile.defaultCronetEngine(); + await client.send(Request('GET', successServerUri.replace(path: '/3')) + ..followRedirects = true + ..maxRedirects = 4); + profile = client.profile!; + + expect(profile.requestData.followRedirects, true); + expect(profile.requestData.maxRedirects, 4); + expect(profile.responseData.isRedirect, false); + + expect(profile.responseData.redirects, [ + HttpProfileRedirectData( + statusCode: 302, + method: 'GET', + location: successServerUri.replace(path: '/2').toString()), + HttpProfileRedirectData( + statusCode: 302, + method: 'GET', + location: successServerUri.replace(path: '/1').toString()), + HttpProfileRedirectData( + statusCode: 302, + method: 'GET', + location: successServerUri.replace(path: '/').toString(), + ) + ]); + }); + + test('no follow redirects', () async { + final client = CronetClientWithProfile.defaultCronetEngine(); + await client.send(Request('GET', successServerUri.replace(path: '/3')) + ..followRedirects = false); + profile = client.profile!; + + expect(profile.requestData.followRedirects, false); + expect(profile.responseData.isRedirect, true); + expect(profile.responseData.redirects, isEmpty); + }); + }); + }); +} diff --git a/pkgs/cronet_http/example/pubspec.yaml b/pkgs/cronet_http/example/pubspec.yaml index 41c6611433..7a258b76eb 100644 --- a/pkgs/cronet_http/example/pubspec.yaml +++ b/pkgs/cronet_http/example/pubspec.yaml @@ -22,6 +22,8 @@ dev_dependencies: sdk: flutter http_client_conformance_tests: path: ../../http_client_conformance_tests/ + http_profile: + path: ../../http_profile/ integration_test: sdk: flutter test: ^1.23.1 diff --git a/pkgs/cronet_http/lib/src/cronet_client.dart b/pkgs/cronet_http/lib/src/cronet_client.dart index c9573826f3..62c3a5d455 100644 --- a/pkgs/cronet_http/lib/src/cronet_client.dart +++ b/pkgs/cronet_http/lib/src/cronet_client.dart @@ -16,6 +16,7 @@ library; import 'dart:async'; import 'package:http/http.dart'; +import 'package:http_profile/http_profile.dart'; import 'package:jni/jni.dart'; import 'jni/jni_bindings.dart' as jb; @@ -157,7 +158,9 @@ Map _cronetToClientHeaders( value.join(','))); jb.UrlRequestCallbackProxy_UrlRequestCallbackInterface _urlRequestCallbacks( - BaseRequest request, Completer responseCompleter) { + BaseRequest request, + Completer responseCompleter, + HttpClientRequestProfile? profile) { StreamController>? responseStream; JByteBuffer? jByteBuffer; var numRedirects = 0; @@ -198,10 +201,22 @@ jb.UrlRequestCallbackProxy_UrlRequestCallbackInterface _urlRequestCallbacks( headers: responseHeaders, )); + profile?.requestData.close(); + profile?.responseData + ?..contentLength = contentLength + ..headersCommaValues = responseHeaders + ..isRedirect = false + ..reasonPhrase = + responseInfo.getHttpStatusText().toDartString(releaseOriginal: true) + ..startTime = DateTime.now() + ..statusCode = responseInfo.getHttpStatusCode(); jByteBuffer = JByteBuffer.allocateDirect(_bufferSize); urlRequest.read(jByteBuffer!); }, onRedirectReceived: (urlRequest, responseInfo, newLocationUrl) { + final responseHeaders = + _cronetToClientHeaders(responseInfo.getAllHeaders()); + if (!request.followRedirects) { urlRequest.cancel(); responseCompleter.complete(StreamedResponse( @@ -214,10 +229,27 @@ jb.UrlRequestCallbackProxy_UrlRequestCallbackInterface _urlRequestCallbacks( request: request, isRedirect: true, headers: _cronetToClientHeaders(responseInfo.getAllHeaders()))); + + profile?.responseData + ?..headersCommaValues = responseHeaders + ..isRedirect = true + ..reasonPhrase = responseInfo + .getHttpStatusText() + .toDartString(releaseOriginal: true) + ..startTime = DateTime.now() + ..statusCode = responseInfo.getHttpStatusCode(); + return; } ++numRedirects; if (numRedirects <= request.maxRedirects) { + profile?.responseData.addRedirect(HttpProfileRedirectData( + statusCode: responseInfo.getHttpStatusCode(), + // This method is not correct for status codes 303 to 307. Cronet + // does not seem to have a way to get the method so we'd have to + // calculate it according to the rules in RFC-7231. + method: 'GET', + location: newLocationUrl.toDartString(releaseOriginal: true))); urlRequest.followRedirect(); } else { urlRequest.cancel(); @@ -227,8 +259,9 @@ jb.UrlRequestCallbackProxy_UrlRequestCallbackInterface _urlRequestCallbacks( }, onReadCompleted: (urlRequest, responseInfo, byteBuffer) { byteBuffer.flip(); - responseStream! - .add(jByteBuffer!.asUint8List().sublist(0, byteBuffer.remaining)); + final data = jByteBuffer!.asUint8List().sublist(0, byteBuffer.remaining); + responseStream!.add(data); + profile?.responseData.bodySink.add(data); byteBuffer.clear(); urlRequest.read(byteBuffer); @@ -236,6 +269,7 @@ jb.UrlRequestCallbackProxy_UrlRequestCallbackInterface _urlRequestCallbacks( onSucceeded: (urlRequest, responseInfo) { responseStream!.sink.close(); jByteBuffer?.release(); + profile?.responseData.close(); }, onFailed: (urlRequest, responseInfo, cronetException) { final error = ClientException( @@ -246,6 +280,14 @@ jb.UrlRequestCallbackProxy_UrlRequestCallbackInterface _urlRequestCallbacks( responseStream!.addError(error); responseStream!.close(); } + + if (profile != null) { + if (profile.requestData.endTime == null) { + profile.requestData.closeWithError(error.toString()); + } else { + profile.responseData.closeWithError(error.toString()); + } + } jByteBuffer?.release(); }, )); @@ -324,6 +366,12 @@ class CronetClient extends BaseClient { _isClosed = true; } + HttpClientRequestProfile? _createProfile(BaseRequest request) => + HttpClientRequestProfile.profile( + requestStartTime: DateTime.now(), + requestMethod: request.method, + requestUri: request.url.toString()); + @override Future send(BaseRequest request) async { if (_isClosed) { @@ -339,14 +387,33 @@ class CronetClient extends BaseClient { 'HTTP request failed. CronetEngine is already closed.', request.url); } + final profile = _createProfile(request); + profile?.connectionInfo = { + 'package': 'package:cronet_http', + 'client': 'CronetHttp', + }; + profile?.requestData + ?..contentLength = request.contentLength + ..followRedirects = request.followRedirects + ..headersCommaValues = request.headers + ..maxRedirects = request.maxRedirects; + if (profile != null && request.contentLength != null) { + profile.requestData.headersListValues = { + 'Content-Length': ['${request.contentLength}'], + ...profile.requestData.headers! + }; + } + final stream = request.finalize(); final body = await stream.toBytes(); + profile?.requestData.bodySink.add(body); + final responseCompleter = Completer(); final builder = engine._engine.newUrlRequestBuilder( request.url.toString().toJString(), jb.UrlRequestCallbackProxy.new1( - _urlRequestCallbacks(request, responseCompleter)), + _urlRequestCallbacks(request, responseCompleter, profile)), _executor, )..setHttpMethod(request.method.toJString()); @@ -367,3 +434,17 @@ class CronetClient extends BaseClient { return responseCompleter.future; } } + +/// A test-only class that makes the [HttpClientRequestProfile] data available. +class CronetClientWithProfile extends CronetClient { + HttpClientRequestProfile? profile; + + @override + HttpClientRequestProfile? _createProfile(BaseRequest request) => + profile = super._createProfile(request); + + CronetClientWithProfile._(super._engine, super._closeEngine) : super._(); + + factory CronetClientWithProfile.defaultCronetEngine() => + CronetClientWithProfile._(null, true); +} diff --git a/pkgs/cronet_http/pubspec.yaml b/pkgs/cronet_http/pubspec.yaml index 69af73d277..00d0be576f 100644 --- a/pkgs/cronet_http/pubspec.yaml +++ b/pkgs/cronet_http/pubspec.yaml @@ -3,6 +3,7 @@ version: 1.2.0 description: >- An Android Flutter plugin that provides access to the Cronet HTTP client. repository: https://github.com/dart-lang/http/tree/master/pkgs/cronet_http +publish_to: none environment: sdk: ^3.0.0 @@ -12,6 +13,8 @@ dependencies: flutter: sdk: flutter http: ^1.2.0 + http_profile: + path: ../http_profile/ jni: ^0.7.3 dev_dependencies: From 35d5f77beaf905000d90d597b53261d633696421 Mon Sep 17 00:00:00 2001 From: Brian Quinlan Date: Fri, 29 Mar 2024 11:59:21 -0700 Subject: [PATCH 2/4] Update client_test.dart --- .../example/integration_test/client_test.dart | 39 +++++++++++++++---- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/pkgs/cronet_http/example/integration_test/client_test.dart b/pkgs/cronet_http/example/integration_test/client_test.dart index f126f84fc3..e2ea74912a 100644 --- a/pkgs/cronet_http/example/integration_test/client_test.dart +++ b/pkgs/cronet_http/example/integration_test/client_test.dart @@ -4,18 +4,41 @@ import 'package:cronet_http/cronet_http.dart'; import 'package:http_client_conformance_tests/http_client_conformance_tests.dart'; +import 'package:http_profile/http_profile.dart'; import 'package:integration_test/integration_test.dart'; import 'package:test/test.dart'; Future testConformance() async { - group( - 'default cronet engine', - () => testAll( - CronetClient.defaultCronetEngine, - canStreamRequestBody: false, - canReceiveSetCookieHeaders: true, - canSendCookieHeaders: true, - )); + group('default cronet engine', () { + group('profile enabled', () { + final profile = HttpClientRequestProfile.profilingEnabled; + HttpClientRequestProfile.profilingEnabled = true; + try { + testAll( + CronetClient.defaultCronetEngine, + canStreamRequestBody: false, + canReceiveSetCookieHeaders: true, + canSendCookieHeaders: true, + ); + } finally { + HttpClientRequestProfile.profilingEnabled = profile; + } + }); + group('profile disabled', () { + final profile = HttpClientRequestProfile.profilingEnabled; + HttpClientRequestProfile.profilingEnabled = false; + try { + testAll( + CronetClient.defaultCronetEngine, + canStreamRequestBody: false, + canReceiveSetCookieHeaders: true, + canSendCookieHeaders: true, + ); + } finally { + HttpClientRequestProfile.profilingEnabled = profile; + } + }); + }); group('from cronet engine', () { testAll( From e6d674c7dc455377b43820c79a9a50cb5a4cc94f Mon Sep 17 00:00:00 2001 From: Brian Quinlan Date: Thu, 9 May 2024 14:59:48 -0700 Subject: [PATCH 3/4] Ready --- pkgs/cronet_http/CHANGELOG.md | 5 +++++ pkgs/cronet_http/example/pubspec.yaml | 3 +-- pkgs/cronet_http/pubspec.yaml | 10 ++++------ 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/pkgs/cronet_http/CHANGELOG.md b/pkgs/cronet_http/CHANGELOG.md index 1cc924894e..5848fbaaa9 100644 --- a/pkgs/cronet_http/CHANGELOG.md +++ b/pkgs/cronet_http/CHANGELOG.md @@ -1,3 +1,8 @@ +## 1.3.0-wip + +* Add integration to the + [DevTools "Network" tab](https://docs.flutter.dev/tools/devtools/network). + ## 1.2.0 * Support the Cronet embedding dependency with `--dart-define=cronetHttpNoPlay=true`. diff --git a/pkgs/cronet_http/example/pubspec.yaml b/pkgs/cronet_http/example/pubspec.yaml index 7a258b76eb..5447d82b58 100644 --- a/pkgs/cronet_http/example/pubspec.yaml +++ b/pkgs/cronet_http/example/pubspec.yaml @@ -22,8 +22,7 @@ dev_dependencies: sdk: flutter http_client_conformance_tests: path: ../../http_client_conformance_tests/ - http_profile: - path: ../../http_profile/ + http_profile: ^0.1.0 integration_test: sdk: flutter test: ^1.23.1 diff --git a/pkgs/cronet_http/pubspec.yaml b/pkgs/cronet_http/pubspec.yaml index e4e94f4ede..b568ae548a 100644 --- a/pkgs/cronet_http/pubspec.yaml +++ b/pkgs/cronet_http/pubspec.yaml @@ -1,20 +1,18 @@ name: cronet_http -version: 1.2.0 +version: 1.3.0-wip description: >- An Android Flutter plugin that provides access to the Cronet HTTP client. repository: https://github.com/dart-lang/http/tree/master/pkgs/cronet_http -publish_to: none environment: - sdk: ^3.0.0 - flutter: ">=3.0.0" + sdk: ^3.4.0 + flutter: '>=3.22.0' dependencies: flutter: sdk: flutter http: ^1.2.0 - http_profile: - path: ../http_profile/ + http_profile: ^0.1.0 jni: ^0.8.0 dev_dependencies: From 47c1995ac38197f26f0c5a4a65a32ed5dfb24e3b Mon Sep 17 00:00:00 2001 From: Brian Quinlan Date: Wed, 22 May 2024 17:00:40 -0700 Subject: [PATCH 4/4] Review updates --- pkgs/cronet_http/CHANGELOG.md | 2 +- pkgs/cronet_http/example/pubspec.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkgs/cronet_http/CHANGELOG.md b/pkgs/cronet_http/CHANGELOG.md index 0bb8858874..fd2acca034 100644 --- a/pkgs/cronet_http/CHANGELOG.md +++ b/pkgs/cronet_http/CHANGELOG.md @@ -1,7 +1,7 @@ ## 1.3.0 * Add integration to the - [DevTools "Network" tab](https://docs.flutter.dev/tools/devtools/network). + [DevTools Network View](https://docs.flutter.dev/tools/devtools/network). ## 1.2.1 diff --git a/pkgs/cronet_http/example/pubspec.yaml b/pkgs/cronet_http/example/pubspec.yaml index 5447d82b58..122e2833a3 100644 --- a/pkgs/cronet_http/example/pubspec.yaml +++ b/pkgs/cronet_http/example/pubspec.yaml @@ -4,7 +4,7 @@ description: Demonstrates how to use the cronet_http plugin. publish_to: 'none' environment: - sdk: ^3.2.0 + sdk: ^3.4.0 dependencies: cronet_http: