From 8c020b33221855f534c4671b028c377b7b8b9445 Mon Sep 17 00:00:00 2001 From: Brian Quinlan Date: Tue, 20 Feb 2024 09:41:43 -0800 Subject: [PATCH 1/4] Release the CronetEngine when it is shutdown --- pkgs/cronet_http/lib/src/cronet_client.dart | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/pkgs/cronet_http/lib/src/cronet_client.dart b/pkgs/cronet_http/lib/src/cronet_client.dart index 3c2a4264d4..b355fd7c97 100644 --- a/pkgs/cronet_http/lib/src/cronet_client.dart +++ b/pkgs/cronet_http/lib/src/cronet_client.dart @@ -49,6 +49,7 @@ enum CacheMode { /// An environment that can be used to make HTTP requests. class CronetEngine { late final jb.CronetEngine _engine; + bool _isClosed = false; CronetEngine._(this._engine); @@ -140,7 +141,11 @@ class CronetEngine { } void close() { - _engine.shutdown(); + if (!_isClosed) { + _engine.shutdown(); + _engine.release(); + } + _isClosed = true; } } From a81ecf6eb21c448cde10fdcbab2b43e8c8d02bd5 Mon Sep 17 00:00:00 2001 From: Brian Quinlan Date: Tue, 20 Feb 2024 10:59:42 -0800 Subject: [PATCH 2/4] 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)), From 3ca9f5602dce57faa74e9888c38db68d11435852 Mon Sep 17 00:00:00 2001 From: Brian Quinlan Date: Tue, 20 Feb 2024 11:09:13 -0800 Subject: [PATCH 3/4] Update pubspec.yaml --- pkgs/cronet_http/pubspec.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkgs/cronet_http/pubspec.yaml b/pkgs/cronet_http/pubspec.yaml index f1a642bbb3..f3cb350ebe 100644 --- a/pkgs/cronet_http/pubspec.yaml +++ b/pkgs/cronet_http/pubspec.yaml @@ -1,5 +1,5 @@ name: cronet_http -version: 1.1.0 +version: 1.1.1 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 From e951f15d27c2ca04b7777bed7c7b49d83a27f688 Mon Sep 17 00:00:00 2001 From: Brian Quinlan Date: Wed, 21 Feb 2024 18:35:47 -0800 Subject: [PATCH 4/4] s/isOwned/closeEngine/g --- .../cronet_configuration_test.dart | 4 ++-- pkgs/cronet_http/example/lib/main.dart | 2 +- pkgs/cronet_http/lib/src/cronet_client.dart | 16 ++++++++-------- 3 files changed, 11 insertions(+), 11 deletions(-) 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 7fecbe4ca2..63ce8eba0f 100644 --- a/pkgs/cronet_http/example/integration_test/cronet_configuration_test.dart +++ b/pkgs/cronet_http/example/integration_test/cronet_configuration_test.dart @@ -138,12 +138,12 @@ void testEngineClose() { test('engine owned close', () { final engine = CronetEngine.build(); - CronetClient.fromCronetEngine(engine, isOwned: true).close(); + CronetClient.fromCronetEngine(engine, closeEngine: true).close(); }); test('engine not owned close', () { final engine = CronetEngine.build(); - CronetClient.fromCronetEngine(engine, isOwned: false).close(); + CronetClient.fromCronetEngine(engine, closeEngine: false).close(); engine.close(); }); }); diff --git a/pkgs/cronet_http/example/lib/main.dart b/pkgs/cronet_http/example/lib/main.dart index 02243ded3f..02c56d6581 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, isOwned: true); + httpClient = CronetClient.fromCronetEngine(engine, closeEngine: 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 9cc2fe47a1..c9573826f3 100644 --- a/pkgs/cronet_http/lib/src/cronet_client.dart +++ b/pkgs/cronet_http/lib/src/cronet_client.dart @@ -282,9 +282,9 @@ class CronetClient extends BaseClient { bool _isClosed = false; /// Indicates that [CronetClient] is responsible for closing [_engine]. - final bool _ownedEngine; + final bool _closeEngine; - CronetClient._(this._engine, this._ownedEngine) { + CronetClient._(this._engine, this._closeEngine) { Jni.initDLApi(); } @@ -293,12 +293,12 @@ class CronetClient extends BaseClient { /// A [CronetClient] configured with a [CronetEngine]. /// - /// 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]. + /// If [closeEngine] is `true`, then [engine] will be closed when [close] is + /// called on this [CronetClient]. This can simplify lifetime management if + /// [engine] is only used in one [CronetClient]. factory CronetClient.fromCronetEngine(CronetEngine engine, - {bool isOwned = false}) => - CronetClient._(engine, isOwned); + {bool closeEngine = false}) => + CronetClient._(engine, closeEngine); /// A [CronetClient] configured with a [Future] containing a [CronetEngine]. /// @@ -318,7 +318,7 @@ class CronetClient extends BaseClient { /// ``` @override void close() { - if (!_isClosed && _ownedEngine) { + if (!_isClosed && _closeEngine) { _engine?.close(); } _isClosed = true;