From a81ecf6eb21c448cde10fdcbab2b43e8c8d02bd5 Mon Sep 17 00:00:00 2001 From: Brian Quinlan Date: Tue, 20 Feb 2024 10:59:42 -0800 Subject: [PATCH] Add tests, readme, changelog --- pkgs/cronet_http/CHANGELOG.md | 5 ++++ pkgs/cronet_http/README.md | 3 +- .../cronet_configuration_test.dart | 30 +++++++++++++++++++ pkgs/cronet_http/example/lib/main.dart | 2 +- pkgs/cronet_http/lib/src/cronet_client.dart | 29 +++++++++++------- 5 files changed, 57 insertions(+), 12 deletions(-) diff --git a/pkgs/cronet_http/CHANGELOG.md b/pkgs/cronet_http/CHANGELOG.md index bc9255116c..f8b134047d 100644 --- a/pkgs/cronet_http/CHANGELOG.md +++ b/pkgs/cronet_http/CHANGELOG.md @@ -1,3 +1,8 @@ +## 1.1.1 + +* Make it possible to construct `CronetClient` with custom a `CronetEngine` + while still allowing `CronetClient` to close the `CronetEngine`. + ## 1.1.0 * Use `package:http_image_provider` in the example application. diff --git a/pkgs/cronet_http/README.md b/pkgs/cronet_http/README.md index 47ebd8bac1..e53e54d19e 100644 --- a/pkgs/cronet_http/README.md +++ b/pkgs/cronet_http/README.md @@ -43,7 +43,7 @@ void main() async { cacheMode: CacheMode.memory, cacheMaxSize: 2 * 1024 * 1024, userAgent: 'Book Agent'); - httpClient = CronetClient.fromCronetEngine(engine); + httpClient = CronetClient.fromCronetEngine(engine, isOwned: true); } else { httpClient = IOClient(HttpClient()..userAgent = 'Book Agent'); } @@ -52,6 +52,7 @@ void main() async { 'www.googleapis.com', '/books/v1/volumes', {'q': 'HTTP', 'maxResults': '40', 'printType': 'books'})); + httpClient.close(); } ``` diff --git a/pkgs/cronet_http/example/integration_test/cronet_configuration_test.dart b/pkgs/cronet_http/example/integration_test/cronet_configuration_test.dart index f787ebc39a..7fecbe4ca2 100644 --- a/pkgs/cronet_http/example/integration_test/cronet_configuration_test.dart +++ b/pkgs/cronet_http/example/integration_test/cronet_configuration_test.dart @@ -5,6 +5,7 @@ import 'dart:io'; import 'package:cronet_http/cronet_http.dart'; +import 'package:http/http.dart'; import 'package:integration_test/integration_test.dart'; import 'package:test/test.dart'; @@ -120,10 +121,39 @@ void testUserAgent() { }); } +void testEngineClose() { + group('engine close', () { + test('multiple close', () { + CronetEngine.build() + ..close() + ..close(); + }); + + test('request after close', () async { + final closedEngine = CronetEngine.build()..close(); + final client = CronetClient.fromCronetEngine(closedEngine); + await expectLater(() => client.get(Uri.https('example.com', '/')), + throwsA(isA())); + }); + + test('engine owned close', () { + final engine = CronetEngine.build(); + CronetClient.fromCronetEngine(engine, isOwned: true).close(); + }); + + test('engine not owned close', () { + final engine = CronetEngine.build(); + CronetClient.fromCronetEngine(engine, isOwned: false).close(); + engine.close(); + }); + }); +} + void main() { IntegrationTestWidgetsFlutterBinding.ensureInitialized(); testCache(); testInvalidConfigurations(); testUserAgent(); + testEngineClose(); } diff --git a/pkgs/cronet_http/example/lib/main.dart b/pkgs/cronet_http/example/lib/main.dart index 6e1bad22c4..02243ded3f 100644 --- a/pkgs/cronet_http/example/lib/main.dart +++ b/pkgs/cronet_http/example/lib/main.dart @@ -21,7 +21,7 @@ void main() { cacheMode: CacheMode.memory, cacheMaxSize: 2 * 1024 * 1024, userAgent: 'Book Agent'); - httpClient = CronetClient.fromCronetEngine(engine); + httpClient = CronetClient.fromCronetEngine(engine, isOwned: true); } else { httpClient = IOClient(HttpClient()..userAgent = 'Book Agent'); } diff --git a/pkgs/cronet_http/lib/src/cronet_client.dart b/pkgs/cronet_http/lib/src/cronet_client.dart index b355fd7c97..9cc2fe47a1 100644 --- a/pkgs/cronet_http/lib/src/cronet_client.dart +++ b/pkgs/cronet_http/lib/src/cronet_client.dart @@ -142,8 +142,9 @@ class CronetEngine { void close() { if (!_isClosed) { - _engine.shutdown(); - _engine.release(); + _engine + ..shutdown() + ..release(); } _isClosed = true; } @@ -280,9 +281,7 @@ class CronetClient extends BaseClient { CronetEngine? _engine; bool _isClosed = false; - /// Indicates that [_engine] was constructed as an implementation detail for - /// this [CronetClient] (i.e. was not provided as a constructor argument) and - /// should be closed when this [CronetClient] is closed. + /// Indicates that [CronetClient] is responsible for closing [_engine]. final bool _ownedEngine; CronetClient._(this._engine, this._ownedEngine) { @@ -293,8 +292,13 @@ class CronetClient extends BaseClient { factory CronetClient.defaultCronetEngine() => CronetClient._(null, true); /// A [CronetClient] configured with a [CronetEngine]. - factory CronetClient.fromCronetEngine(CronetEngine engine) => - CronetClient._(engine, false); + /// + /// If [isOwned] is `true`, then [engine] will be closed when [close] is + /// called. This can simplify lifetime management if [engine] is only used + /// in one [CronetClient]. + factory CronetClient.fromCronetEngine(CronetEngine engine, + {bool isOwned = false}) => + CronetClient._(engine, isOwned); /// A [CronetClient] configured with a [Future] containing a [CronetEngine]. /// @@ -327,14 +331,19 @@ class CronetClient extends BaseClient { 'HTTP request failed. Client is already closed.', request.url); } - _engine ??= CronetEngine.build(); + final engine = _engine ?? CronetEngine.build(); + _engine = engine; + + if (engine._isClosed) { + throw ClientException( + 'HTTP request failed. CronetEngine is already closed.', request.url); + } final stream = request.finalize(); final body = await stream.toBytes(); final responseCompleter = Completer(); - final engine = _engine!._engine; - final builder = engine.newUrlRequestBuilder( + final builder = engine._engine.newUrlRequestBuilder( request.url.toString().toJString(), jb.UrlRequestCallbackProxy.new1( _urlRequestCallbacks(request, responseCompleter)),