Make it possible for CronetClient to own the lifetime of an existing CronetEngine (#1137)

Author: brianquinlan

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.

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();
 }
 ```
 

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<ClientException>()));
+    });
+
+    test('engine owned close', () {
+      final engine = CronetEngine.build();
+      CronetClient.fromCronetEngine(engine, closeEngine: true).close();
+    });
+
+    test('engine not owned close', () {
+      final engine = CronetEngine.build();
+      CronetClient.fromCronetEngine(engine, closeEngine: false).close();
+      engine.close();
+    });
+  });
+}
+
 void main() {
   IntegrationTestWidgetsFlutterBinding.ensureInitialized();
 
   testCache();
   testInvalidConfigurations();
   testUserAgent();
+  testEngineClose();
 }

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, closeEngine: true);
   } else {
     httpClient = IOClient(HttpClient()..userAgent = 'Book Agent');
   }

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,12 @@ class CronetEngine {
   }
 
   void close() {
-    _engine.shutdown();
+    if (!_isClosed) {
+      _engine
+        ..shutdown()
+        ..release();
+    }
+    _isClosed = true;
   }
 }
 
@@ -275,21 +281,24 @@ 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.
-  final bool _ownedEngine;
+  /// Indicates that [CronetClient] is responsible for closing [_engine].
+  final bool _closeEngine;
 
-  CronetClient._(this._engine, this._ownedEngine) {
+  CronetClient._(this._engine, this._closeEngine) {
     Jni.initDLApi();
   }
 
   /// A [CronetClient] that will be initialized with a new [CronetEngine].
   factory CronetClient.defaultCronetEngine() => CronetClient._(null, true);
 
   /// A [CronetClient] configured with a [CronetEngine].
-  factory CronetClient.fromCronetEngine(CronetEngine engine) =>
-      CronetClient._(engine, false);
+  ///
+  /// 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 closeEngine = false}) =>
+      CronetClient._(engine, closeEngine);
 
   /// A [CronetClient] configured with a [Future] containing a [CronetEngine].
   ///
@@ -309,7 +318,7 @@ class CronetClient extends BaseClient {
   /// ```
   @override
   void close() {
-    if (!_isClosed && _ownedEngine) {
+    if (!_isClosed && _closeEngine) {
       _engine?.close();
     }
     _isClosed = true;
@@ -322,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<StreamedResponse>();
-    final engine = _engine!._engine;
 
-    final builder = engine.newUrlRequestBuilder(
+    final builder = engine._engine.newUrlRequestBuilder(
       request.url.toString().toJString(),
       jb.UrlRequestCallbackProxy.new1(
           _urlRequestCallbacks(request, responseCompleter)),

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