From ab5e2da1dfaa44144df4889a66f98d2411c8f933 Mon Sep 17 00:00:00 2001 From: Andrew Kolos Date: Wed, 5 Jun 2024 11:23:36 -0700 Subject: [PATCH 1/2] [unified_analytics] Suppress any `FileSystemException` thrown during `Analytics.send` (#274) --- pkgs/unified_analytics/CHANGELOG.md | 4 ++++ pkgs/unified_analytics/lib/src/constants.dart | 4 ++-- .../lib/src/log_handler.dart | 23 +++++++++++-------- pkgs/unified_analytics/pubspec.yaml | 2 +- .../test/log_handler_test.dart | 21 +++++++++++++++++ 5 files changed, 42 insertions(+), 12 deletions(-) diff --git a/pkgs/unified_analytics/CHANGELOG.md b/pkgs/unified_analytics/CHANGELOG.md index 98769412..cd8e4b16 100644 --- a/pkgs/unified_analytics/CHANGELOG.md +++ b/pkgs/unified_analytics/CHANGELOG.md @@ -1,3 +1,7 @@ +## 5.8.8+2 + +- Fixed bug where calling `Analytics.send` could result in a `FileSystemException` when unable to write to a log file. + ## 5.8.8+1 - Edit to error handler to not use default `Analytic.send` method and use new `Analytics._sendError` method that doesn't create a session id diff --git a/pkgs/unified_analytics/lib/src/constants.dart b/pkgs/unified_analytics/lib/src/constants.dart index 20c9379e..861d6490 100644 --- a/pkgs/unified_analytics/lib/src/constants.dart +++ b/pkgs/unified_analytics/lib/src/constants.dart @@ -26,7 +26,7 @@ const String kConfigString = ''' # All other lines are configuration lines. They have # the form "name=value". If multiple lines contain # the same configuration name with different values, -# the parser will default to a conservative value. +# the parser will default to a conservative value. # DISABLING TELEMETRY REPORTING # @@ -82,7 +82,7 @@ const int kLogFileLength = 2500; const String kLogFileName = 'dart-flutter-telemetry.log'; /// The current version of the package, should be in line with pubspec version. -const String kPackageVersion = '5.8.8+1'; +const String kPackageVersion = '5.8.8+2'; /// The minimum length for a session. const int kSessionDurationMinutes = 30; diff --git a/pkgs/unified_analytics/lib/src/log_handler.dart b/pkgs/unified_analytics/lib/src/log_handler.dart index c406cce9..726b1466 100644 --- a/pkgs/unified_analytics/lib/src/log_handler.dart +++ b/pkgs/unified_analytics/lib/src/log_handler.dart @@ -285,15 +285,20 @@ class LogHandler { var records = logFile.readAsLinesSync(); final content = '${jsonEncode(data)}\n'; - // When the record count is less than the max, add as normal; - // else drop the oldest records until equal to max - if (records.length < kLogFileLength) { - logFile.writeAsStringSync(content, mode: FileMode.writeOnlyAppend); - } else { - records.add(content); - records = records.skip(records.length - kLogFileLength).toList(); - - logFile.writeAsStringSync(records.join('\n')); + try { + // When the record count is less than the max, add as normal; + // else drop the oldest records until equal to max + if (records.length < kLogFileLength) { + logFile.writeAsStringSync(content, mode: FileMode.writeOnlyAppend); + } else { + records.add(content); + records = records.skip(records.length - kLogFileLength).toList(); + + logFile.writeAsStringSync(records.join('\n')); + } + } on FileSystemException { + // Logging isn't important enough to warrant raising a + // FileSystemException that will surprise consumers of this package. } } } diff --git a/pkgs/unified_analytics/pubspec.yaml b/pkgs/unified_analytics/pubspec.yaml index c2f6916c..d104c4ad 100644 --- a/pkgs/unified_analytics/pubspec.yaml +++ b/pkgs/unified_analytics/pubspec.yaml @@ -4,7 +4,7 @@ description: >- to Google Analytics. # When updating this, keep the version consistent with the changelog and the # value in lib/src/constants.dart. -version: 5.8.8+1 +version: 5.8.8+2 repository: https://github.com/dart-lang/tools/tree/main/pkgs/unified_analytics environment: diff --git a/pkgs/unified_analytics/test/log_handler_test.dart b/pkgs/unified_analytics/test/log_handler_test.dart index cebbd133..bbd8a87e 100644 --- a/pkgs/unified_analytics/test/log_handler_test.dart +++ b/pkgs/unified_analytics/test/log_handler_test.dart @@ -9,6 +9,7 @@ import 'package:test/test.dart'; import 'package:unified_analytics/src/constants.dart'; import 'package:unified_analytics/src/enums.dart'; +import 'package:unified_analytics/src/log_handler.dart'; import 'package:unified_analytics/src/utils.dart'; import 'package:unified_analytics/unified_analytics.dart'; @@ -200,6 +201,26 @@ void main() { // expect(logFile.readAsLinesSync()[0].trim(), isNot('{{')); }); + test( + 'Catches and discards any FileSystemException raised from attempting ' + 'to write to the log file', () async { + final logFilePath = 'log.txt'; + final fs = MemoryFileSystem.test(opHandle: (context, operation) { + if (context == logFilePath && operation == FileSystemOp.write) { + throw FileSystemException( + 'writeFrom failed', + logFilePath, + const OSError('No space left on device', 28), + ); + } + }); + final logFile = fs.file(logFilePath); + logFile.createSync(); + final logHandler = LogHandler(logFile: logFile); + + logHandler.save(data: {}); + }); + test('Catching cast errors for each log record silently', () async { // Write a json array to the log file which will cause // a cast error when parsing each line From 6d41e14e44694f2ecfebc67f4d39b6f3e6af8fe3 Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Mon, 24 Jun 2024 17:03:35 -0700 Subject: [PATCH 2/2] delete old log file if it exceeds kMaxLogFileSize of 25MiB (#277) * delete old log file if it exceeds kMaxLogFileSize of 25MiB * update changelog * bump version in pubspec.yaml * update constants.dart --- pkgs/unified_analytics/CHANGELOG.md | 1 + pkgs/unified_analytics/lib/src/constants.dart | 5 + .../lib/src/log_handler.dart | 14 ++- .../test/log_handler_test.dart | 92 +++++++++++++++++++ 4 files changed, 109 insertions(+), 3 deletions(-) diff --git a/pkgs/unified_analytics/CHANGELOG.md b/pkgs/unified_analytics/CHANGELOG.md index cd8e4b16..0aba8aaf 100644 --- a/pkgs/unified_analytics/CHANGELOG.md +++ b/pkgs/unified_analytics/CHANGELOG.md @@ -1,5 +1,6 @@ ## 5.8.8+2 +- Avoid opening large telemetry log files to prevent out of memory errors. - Fixed bug where calling `Analytics.send` could result in a `FileSystemException` when unable to write to a log file. ## 5.8.8+1 diff --git a/pkgs/unified_analytics/lib/src/constants.dart b/pkgs/unified_analytics/lib/src/constants.dart index 861d6490..9c618d75 100644 --- a/pkgs/unified_analytics/lib/src/constants.dart +++ b/pkgs/unified_analytics/lib/src/constants.dart @@ -78,6 +78,11 @@ const String kGoogleAnalyticsMeasurementId = 'G-04BXPVBCWJ'; /// How many data records to store in the log file. const int kLogFileLength = 2500; +/// The maximum allowed size of the telemetry log file. +/// +/// 25 MiB. +const int kMaxLogFileSize = 25 * (1 << 20); + /// Filename for the log file to persist sent events on user's machine. const String kLogFileName = 'dart-flutter-telemetry.log'; diff --git a/pkgs/unified_analytics/lib/src/log_handler.dart b/pkgs/unified_analytics/lib/src/log_handler.dart index 726b1466..286f4a64 100644 --- a/pkgs/unified_analytics/lib/src/log_handler.dart +++ b/pkgs/unified_analytics/lib/src/log_handler.dart @@ -282,10 +282,18 @@ class LogHandler { /// This will keep the max number of records limited to equal to /// or less than [kLogFileLength] records. void save({required Map data}) { - var records = logFile.readAsLinesSync(); - final content = '${jsonEncode(data)}\n'; - try { + final stat = logFile.statSync(); + List records; + if (stat.size > kMaxLogFileSize) { + logFile.deleteSync(); + logFile.createSync(); + records = []; + } else { + records = logFile.readAsLinesSync(); + } + final content = '${jsonEncode(data)}\n'; + // When the record count is less than the max, add as normal; // else drop the oldest records until equal to max if (records.length < kLogFileLength) { diff --git a/pkgs/unified_analytics/test/log_handler_test.dart b/pkgs/unified_analytics/test/log_handler_test.dart index bbd8a87e..1842cee5 100644 --- a/pkgs/unified_analytics/test/log_handler_test.dart +++ b/pkgs/unified_analytics/test/log_handler_test.dart @@ -2,9 +2,12 @@ // 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:convert'; + import 'package:file/file.dart'; import 'package:file/memory.dart'; import 'package:path/path.dart' as p; +import 'package:test/fake.dart'; import 'package:test/test.dart'; import 'package:unified_analytics/src/constants.dart'; @@ -221,6 +224,47 @@ void main() { logHandler.save(data: {}); }); + test('deletes log file larger than kMaxLogFileSize', () async { + var deletedLargeLogFile = false; + var wroteDataToLogFile = false; + const data = {}; + final logFile = _FakeFile('log.txt') + .._deleteSyncImpl = (() => deletedLargeLogFile = true) + .._createSyncImpl = () {} + .._statSyncImpl = (() => _FakeFileStat(kMaxLogFileSize + 1)) + .._writeAsStringSync = (contents, {mode = FileMode.append}) { + expect(contents.trim(), data.toString()); + expect(mode, FileMode.writeOnlyAppend); + wroteDataToLogFile = true; + }; + final logHandler = LogHandler(logFile: logFile); + + logHandler.save(data: data); + expect(deletedLargeLogFile, isTrue); + expect(wroteDataToLogFile, isTrue); + }); + + test('does not delete log file if smaller than kMaxLogFileSize', () async { + var wroteDataToLogFile = false; + const data = {}; + final logFile = _FakeFile('log.txt') + .._deleteSyncImpl = + (() => fail('called logFile.deleteSync() when file was less than ' + 'kMaxLogFileSize')) + .._createSyncImpl = () {} + .._readAsLinesSyncImpl = (() => ['three', 'previous', 'lines']) + .._statSyncImpl = (() => _FakeFileStat(kMaxLogFileSize - 1)) + .._writeAsStringSync = (contents, {mode = FileMode.append}) { + expect(contents.trim(), data.toString()); + expect(mode, FileMode.writeOnlyAppend); + wroteDataToLogFile = true; + }; + final logHandler = LogHandler(logFile: logFile); + + logHandler.save(data: data); + expect(wroteDataToLogFile, isTrue); + }); + test('Catching cast errors for each log record silently', () async { // Write a json array to the log file which will cause // a cast error when parsing each line @@ -301,3 +345,51 @@ void main() { expect(newString, testString); }); } + +class _FakeFileStat extends Fake implements FileStat { + _FakeFileStat(this.size); + + @override + final int size; +} + +class _FakeFile extends Fake implements File { + _FakeFile(this.path); + + List Function()? _readAsLinesSyncImpl; + + @override + List readAsLinesSync({Encoding encoding = utf8}) => + _readAsLinesSyncImpl!(); + + @override + final String path; + + FileStat Function()? _statSyncImpl; + + @override + FileStat statSync() => _statSyncImpl!(); + + void Function()? _deleteSyncImpl; + + @override + void deleteSync({bool recursive = false}) => _deleteSyncImpl!(); + + void Function()? _createSyncImpl; + + @override + void createSync({bool recursive = false, bool exclusive = false}) { + return _createSyncImpl!(); + } + + void Function(String contents, {FileMode mode})? _writeAsStringSync; + + @override + void writeAsStringSync( + String contents, { + FileMode mode = FileMode.write, + Encoding encoding = utf8, + bool flush = false, + }) => + _writeAsStringSync!(contents, mode: mode); +}