Fix and test cover case of customized gcCountBuffer (#99)

dart-lang · Jul 21, 2023 · 6e3f57c · 6e3f57c
1 parent 2152aab
commit 6e3f57c
Show file tree

Hide file tree

Showing 11 changed files with 207 additions and 168 deletions.
diff --git a/README.md b/README.md
@@ -25,3 +25,11 @@ Documentation:
 | [leak_tracker](pkgs/leak_tracker/) | A framework for memory leak tracking for Dart and Flutter applications. | [![pub package](https://img.shields.io/pub/v/leak_tracker.svg)](https://pub.dev/packages/leak_tracker) |
 | [leak_tracker_flutter_test](pkgs/leak_tracker_flutter_test/) | Tests for leak_tracker that depend on Flutter framework. |  |
 | [leak_tracker_testing](pkgs/leak_tracker_testing/) | Leak tracking code intended for usage in tests. | [![pub package](https://img.shields.io/pub/v/leak_tracker_testing.svg)](https://pub.dev/packages/leak_tracker_testing) |
+
+## How to enable logs
+
+To temporary enable logs, add this line to `main`:
+
+```
+Logger.root.onRecord.listen((LogRecord record) => print(record.message));
+```
diff --git a/pkgs/leak_tracker/CHANGELOG.md b/pkgs/leak_tracker/CHANGELOG.md
@@ -1,3 +1,7 @@
+# 8.0.3
+
+* Fix an issue with custom gcCountBuffer values.
+
 # 8.0.2
 
 * Improve performance.

diff --git a/pkgs/leak_tracker/lib/src/leak_tracking/_object_tracker.dart b/pkgs/leak_tracker/lib/src/leak_tracking/_object_tracker.dart
@@ -222,6 +222,7 @@ class ObjectTracker implements LeakProvider {
 
   Future<void> _addRetainingPath(List<int> objectsToGetPath) async {
     final connection = await connect();
+
     final pathSetters = objectsToGetPath.map((code) async {
       final record = _objects.notGCed[code]!;
       final path =
@@ -230,8 +231,11 @@ class ObjectTracker implements LeakProvider {
         record.setContext(ContextKeys.retainingPath, path);
       }
     });
-    await Future.wait(pathSetters);
-    disconnect();
+
+    await Future.wait(
+      pathSetters,
+      eagerError: true,
+    );
   }
 
   ObjectRecord _notGCed(int code) {

diff --git a/pkgs/leak_tracker/lib/src/leak_tracking/orchestration.dart b/pkgs/leak_tracker/lib/src/leak_tracking/orchestration.dart
@@ -5,13 +5,17 @@
 import 'dart:async';
 import 'dart:developer';
 
+import 'package:logging/logging.dart';
+
 import '../shared/shared_model.dart';
 import '_formatting.dart';
 import 'leak_tracker.dart';
 import 'leak_tracker_model.dart';
 import 'retaining_path/_connection.dart';
 import 'retaining_path/_retaining_path.dart';
 
+final _log = Logger('orchestration.dart');
+
 /// Asynchronous callback.
 ///
 /// The prefix `Dart` is used to avoid conflict with Flutter's [AsyncCallback].
@@ -80,12 +84,21 @@ Future<Leaks> withLeakTracking(
   AsyncCodeRunner? asyncCodeRunner,
   int gcCountBuffer = defaultGcCountBuffer,
 }) async {
+  if (gcCountBuffer <= 0) {
+    throw ArgumentError.value(
+      gcCountBuffer,
+      'gcCountBuffer',
+      'Must be positive.',
+    );
+  }
+
   if (callback == null) return Leaks({});
 
   enableLeakTracking(
     resetIfAlreadyEnabled: true,
     config: LeakTrackingConfiguration.passive(
       leakDiagnosticConfig: leakDiagnosticConfig,
+      gcCountBuffer: gcCountBuffer,
     ),
   );
 
@@ -108,9 +121,7 @@ Future<Leaks> withLeakTracking(
           fullGcCycles: gcCountBuffer,
           timeout: timeoutForFinalGarbageCollection,
         );
-
         leaks = await collectLeaks();
-
         if ((leaks?.total ?? 0) > 0 && shouldThrowOnLeaks) {
           // `expect` should not be used here, because, when the method is used
           // from Flutter, the packages `test` and `flutter_test` conflict.
@@ -144,6 +155,7 @@ Future<void> forceGC({
   Duration? timeout,
   int fullGcCycles = 1,
 }) async {
+  _log.info('Forcing garbage collection with fullGcCycles = $fullGcCycles...');
   final Stopwatch? stopwatch = timeout == null ? null : (Stopwatch()..start());
   final int barrier = reachabilityBarrier;
 
@@ -163,6 +175,7 @@ Future<void> forceGC({
     await Future<void>.delayed(Duration.zero);
     allocateMemory();
   }
+  _log.info('Done forcing garbage collection.');
 }
 
 /// Returns nicely formatted retaining path for the [ref.target].
@@ -181,7 +194,6 @@ Future<String?> formattedRetainingPath(WeakReference ref) async {
     ref.target.runtimeType,
     identityHashCode(ref.target),
   );
-  disconnect();
 
   if (path == null) return null;
   return retainingPathToString(path);

diff --git a/pkgs/leak_tracker/lib/src/leak_tracking/retaining_path/_connection.dart b/pkgs/leak_tracker/lib/src/leak_tracking/retaining_path/_connection.dart
@@ -18,19 +18,9 @@ class Connection {
   final VmService service;
 }
 
-Completer<Connection>? _completer;
-
-void disconnect() => _completer = null;
-
 Future<Connection> connect() async {
-  if (_completer != null) {
-    return await _completer!.future;
-  }
-
   _log.info('Connecting to vm service protocol...');
 
-  final completer = _completer = Completer<Connection>();
-
   final info = await Service.getInfo();
 
   final uri = info.serverWebSocketUri;
@@ -46,11 +36,14 @@ Future<Connection> connect() async {
   final isolates = await _getTwoIsolates(service);
 
   final result = Connection(service, isolates);
-  completer.complete(result);
+  _log.info('Connected to vm service protocol.');
   return result;
 }
 
-void _handleError(Object? error) => throw error ?? Exception('Unknown error');
+void _handleError(Object? error) {
+  _log.info('Error in vm service protocol: $error');
+  throw error ?? Exception('Unknown error');
+}
 
 /// Tries to wait for two isolates to be available.
 ///

diff --git a/pkgs/leak_tracker/pubspec.yaml b/pkgs/leak_tracker/pubspec.yaml
@@ -1,5 +1,5 @@
 name: leak_tracker
-version: 8.0.2
+version: 8.0.3
 description: A framework for memory leak tracking for Dart and Flutter applications.
 repository: https://github.com/dart-lang/leak_tracker/tree/main/pkgs/leak_tracker
 

diff --git a/pkgs/leak_tracker/test/debug/leak_tracking/end_to_end_test.dart b/pkgs/leak_tracker/test/debug/leak_tracking/end_to_end_test.dart
@@ -16,76 +16,82 @@ void main() {
 
   tearDown(() => disableLeakTracking());
 
-  test('Leak tracker respects maxRequestsForRetainingPath.', () async {
-    LeakTrackerGlobalSettings.maxRequestsForRetainingPath = 2;
-    final leaks = await withLeakTracking(
-      () async {
-        LeakingClass();
-        LeakingClass();
-        LeakingClass();
-      },
-      shouldThrowOnLeaks: false,
-      leakDiagnosticConfig: const LeakDiagnosticConfig(
-        collectRetainingPathForNonGCed: true,
-      ),
-    );
+  for (var gcCountBuffer in [1, defaultGcCountBuffer]) {
+    test('Leak tracker respects maxRequestsForRetainingPath, $gcCountBuffer.',
+        () async {
+      LeakTrackerGlobalSettings.maxRequestsForRetainingPath = 2;
+      final leaks = await withLeakTracking(
+        () async {
+          LeakingClass();
+          LeakingClass();
+          LeakingClass();
+        },
+        shouldThrowOnLeaks: false,
+        leakDiagnosticConfig: const LeakDiagnosticConfig(
+          collectRetainingPathForNonGCed: true,
+        ),
+        gcCountBuffer: gcCountBuffer,
+      );
 
-    const pathHeader = '  path: >';
+      const pathHeader = '  path: >';
 
-    expect(leaks.notGCed, hasLength(3));
-    expect(
-      () => expect(leaks, isLeakFree),
-      throwsA(
-        predicate(
-          (e) {
-            if (e is! TestFailure) {
-              throw 'Unexpected exception type: ${e.runtimeType}';
-            }
-            expect(pathHeader.allMatches(e.message!), hasLength(2));
-            return true;
-          },
+      expect(leaks.notGCed, hasLength(3));
+      expect(
+        () => expect(leaks, isLeakFree),
+        throwsA(
+          predicate(
+            (e) {
+              if (e is! TestFailure) {
+                throw 'Unexpected exception type: ${e.runtimeType}';
+              }
+              expect(pathHeader.allMatches(e.message!), hasLength(2));
+              return true;
+            },
+          ),
         ),
-      ),
-    );
-  });
+      );
+    });
 
-  test('Retaining path for not GCed object is reported.', () async {
-    final leaks = await withLeakTracking(
-      () async {
-        LeakingClass();
-      },
-      shouldThrowOnLeaks: false,
-      leakDiagnosticConfig: const LeakDiagnosticConfig(
-        collectRetainingPathForNonGCed: true,
-      ),
-    );
+    test('Retaining path for not GCed object is reported, $gcCountBuffer.',
+        () async {
+      final leaks = await withLeakTracking(
+        () async {
+          LeakingClass();
+        },
+        shouldThrowOnLeaks: false,
+        leakDiagnosticConfig: const LeakDiagnosticConfig(
+          collectRetainingPathForNonGCed: true,
+        ),
+        gcCountBuffer: gcCountBuffer,
+      );
 
-    const expectedRetainingPathTails = [
-      '/leak_tracker/test/test_infra/data/dart_classes.dart/_notGCedObjects',
-      'dart.core/_GrowableList:',
-      '/leak_tracker/test/test_infra/data/dart_classes.dart/LeakTrackedClass',
-    ];
+      const expectedRetainingPathTails = [
+        '/leak_tracker/test/test_infra/data/dart_classes.dart/_notGCedObjects',
+        'dart.core/_GrowableList:',
+        '/leak_tracker/test/test_infra/data/dart_classes.dart/LeakTrackedClass',
+      ];
 
-    expect(leaks.total, 2);
-    expect(
-      () => expect(leaks, isLeakFree),
-      throwsA(
-        predicate(
-          (e) {
-            if (e is! TestFailure) {
-              throw 'Unexpected exception type: ${e.runtimeType}';
-            }
-            _verifyRetainingPath(expectedRetainingPathTails, e.message!);
-            return true;
-          },
+      expect(leaks.total, 2);
+      expect(
+        () => expect(leaks, isLeakFree),
+        throwsA(
+          predicate(
+            (e) {
+              if (e is! TestFailure) {
+                throw 'Unexpected exception type: ${e.runtimeType}';
+              }
+              _verifyRetainingPath(expectedRetainingPathTails, e.message!);
+              return true;
+            },
+          ),
         ),
-      ),
-    );
+      );
 
-    final theLeak = leaks.notGCed.first;
-    expect(theLeak.trackedClass, contains(LeakTrackedClass.library));
-    expect(theLeak.trackedClass, contains('$LeakTrackedClass'));
-  });
+      final theLeak = leaks.notGCed.first;
+      expect(theLeak.trackedClass, contains(LeakTrackedClass.library));
+      expect(theLeak.trackedClass, contains('$LeakTrackedClass'));
+    });
+  }
 }
 
 void _verifyRetainingPath(

diff --git a/pkgs/leak_tracker/test/debug/leak_tracking/retaining_path/_retaining_path_test.dart b/pkgs/leak_tracker/test/debug/leak_tracking/retaining_path/_retaining_path_test.dart
@@ -28,7 +28,6 @@ void main() {
 
   setUp(() {
     _logs.clear();
-    disconnect();
   });
 
   tearDownAll(() async {
@@ -44,7 +43,7 @@ void main() {
       instance.runtimeType,
       identityHashCode(instance),
     );
-    disconnect();
+
     expect(path!.elements, isNotEmpty);
   });
 
@@ -57,11 +56,10 @@ void main() {
       instance.runtimeType,
       identityHashCode(instance),
     );
-    disconnect();
     expect(path!.elements, isNotEmpty);
   });
 
-  test('Connection is happening just once', () async {
+  test('Connection can be reused', () async {
     final instance1 = MyClass();
     final instance2 = MyClass();
     final connection = await connect();
@@ -72,7 +70,6 @@ void main() {
     ];
 
     await Future.wait(obtainers);
-    disconnect();
 
     expect(
       _logs.where((item) => item == 'Connecting to vm service protocol...'),

diff --git a/pkgs/leak_tracker/test/release/README.md b/pkgs/leak_tracker/test/release/README.md
@@ -0,0 +1 @@
+Bots run tests in this folder with 'dart test', where asserts are on, but VM service is unavailable.
diff --git a/pkgs/leak_tracker/test/release/leak_tracking/_leak_checker_test.dart b/pkgs/leak_tracker/test/release/leak_tracking/_leak_checker_test.dart
@@ -17,7 +17,8 @@ class _SummaryValues {
     LeakType.notGCed: 3,
   });
 
-  static final nonZeroCopy = LeakSummary(<LeakType, int>{}..addAll(nonZero.totals));
+  static final nonZeroCopy =
+      LeakSummary(<LeakType, int>{}..addAll(nonZero.totals));
 }
 
 void main() {