fix(secure_storage): process file system operations one at a time on …

…Windows (#5195)

* fix(secure_storage): process fs events in the order they are received

* chore: clear file when it is corrupted

* chore: add test for stale data during parallel writes

* chore: add visibleForTesting annotations

* chore: add tests for in-memory fs and local fs

* chore: remove print from test

packages/secure_storage/amplify_secure_storage_dart/lib/src/utils/file_key_value_store.dart

@@ -1,9 +1,11 @@
 // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
+import 'dart:async';
 import 'dart:convert';
 import 'dart:io';
 
+import 'package:aws_common/aws_common.dart';
 import 'package:file/file.dart' as pkg_file;
 import 'package:file/local.dart' as local_file;
 import 'package:meta/meta.dart';
@@ -18,7 +20,7 @@ import 'package:path/path.dart' as pkg_path;
 // without bringing in flutter as a dependency to the tests.
 class FileKeyValueStore {
   /// {@macro amplify_secure_storage_dart.file_key_value_store}
-  const FileKeyValueStore({
+  FileKeyValueStore({
     required this.path,
     required this.fileName,
     this.fs = const local_file.LocalFileSystem(),
@@ -32,27 +34,23 @@ class FileKeyValueStore {
   /// The file will be created if it does not yet exist.
   final String fileName;
 
+  final TaskScheduler _scheduler = TaskScheduler();
+
+  final AWSLogger logger = AWSLogger('FileKeyValueStore');
+
   @visibleForTesting
   final pkg_file.FileSystem fs;
 
+  @visibleForTesting
   File get file => fs.file(
         pkg_path.join(
           path,
           fileName,
         ),
       );
 
-  /// Writes a single key to storage.
-  Future<void> writeKey({
-    required String key,
-    required Object value,
-  }) async {
-    final data = await readAll();
-    data[key] = value;
-    return writeAll(data);
-  }
-
-  /// Overwrites the existing data.
+  /// Overwrites the existing data in [file] with the key-value pairs in [data].
+  @visibleForTesting
   Future<void> writeAll(
     Map<String, Object> data,
   ) async {
@@ -63,42 +61,104 @@ class FileKeyValueStore {
     await file.writeAsString(stringMap);
   }
 
+  /// Reads all the key-value pairs from [file].
+  @visibleForTesting
+  Future<Map<String, Object>> readAll() async {
+    if (await file.exists()) {
+      final stringMap = await file.readAsString();
+      if (stringMap.isNotEmpty) {
+        try {
+          final Object? data = json.decode(stringMap);
+          if (data is Map) {
+            return data.cast<String, Object>();
+          }
+        } on FormatException catch (e) {
+          logger.error(
+            'Cannot read file. The file may be corrupted. '
+            'Clearing file contents. See error for more details. '
+            'Error: $e',
+          );
+          await writeAll({});
+        }
+      }
+    }
+    return <String, Object>{};
+  }
+
+  /// Writes a single key to storage.
+  Future<void> writeKey({
+    required String key,
+    required Object value,
+  }) async {
+    return _scheduler.schedule(() async {
+      final data = await readAll();
+      data[key] = value;
+      return writeAll(data);
+    });
+  }
+
   /// Reads a single key from storage.
   Future<Object?> readKey({
     required String key,
   }) async {
-    final data = await readAll();
-    return data[key];
+    return _scheduler.schedule(() async {
+      final data = await readAll();
+      return data[key];
+    });
   }
 
   /// Removes a single key from storage.
   Future<void> removeKey({
     required String key,
   }) async {
-    final data = await readAll();
-    data.remove(key);
-    await writeAll(data);
+    return _scheduler.schedule(() async {
+      final data = await readAll();
+      data.remove(key);
+      await writeAll(data);
+    });
   }
 
   /// Returns true if the key exists in storage
   Future<bool> containsKey({
     required String key,
   }) async {
-    final data = await readAll();
-    return data.containsKey(key);
+    return _scheduler.schedule(() async {
+      final data = await readAll();
+      return data.containsKey(key);
+    });
+  }
+}
+
+/// A class for processing async tasks one at a time in the order that they are
+/// scheduled.
+class TaskScheduler {
+  final _taskQueue = <Task<dynamic>>[];
+  bool _isProcessing = false;
+  Future<T> schedule<T>(Future<T> Function() task) {
+    final completer = Completer<T>();
+    _taskQueue.add(Task(task, completer));
+    _processTasks();
+    return completer.future;
   }
 
-  /// Reads all the key-value pairs from storage.
-  Future<Map<String, Object>> readAll() async {
-    if (await file.exists()) {
-      final stringMap = await file.readAsString();
-      if (stringMap.isNotEmpty) {
-        final Object? data = json.decode(stringMap);
-        if (data is Map) {
-          return data.cast<String, Object>();
-        }
+  Future<void> _processTasks() async {
+    if (_isProcessing) return;
+    _isProcessing = true;
+    while (_taskQueue.isNotEmpty) {
+      final currentTask = _taskQueue.removeAt(0);
+      try {
+        final result = await currentTask.task();
+        currentTask.completer.complete(result);
+      } on Object catch (e, stackTrace) {
+        currentTask.completer.completeError(e, stackTrace);
       }
     }
-    return <String, Object>{};
+    _isProcessing = false;
   }
 }
+
+class Task<T> {
+  Task(this.task, this.completer);
+  final Future<T> Function() task;
+  final Completer<T> completer;
+}

packages/secure_storage/amplify_secure_storage_test/test/file_key_value_store_test.dart

@@ -3,83 +3,145 @@
 
 @TestOn('vm')
 
+import 'dart:io';
+
 import 'package:amplify_secure_storage_dart/src/utils/file_key_value_store.dart';
+import 'package:file/local.dart';
 import 'package:file/memory.dart';
 import 'package:test/test.dart';
 
 void main() {
   late FileKeyValueStore storage;
-  group('FileKeyValueStore', () {
-    setUp(() {
-      storage = FileKeyValueStore(
-        path: 'path',
-        fileName: 'file',
-        fs: MemoryFileSystem(),
-      );
-    });
 
-    test('readKey & writeKey', () async {
-      // assert initial state is null
-      final value1 = await storage.readKey(key: 'key');
-      expect(value1, isNull);
+  final fileSystems = [MemoryFileSystem(), const LocalFileSystem()];
 
-      // write key-value pair
-      await storage.writeKey(key: 'key', value: 'value');
+  for (final fileSystem in fileSystems) {
+    group('FileKeyValueStore (${fileSystem.runtimeType})', () {
+      setUp(() {
+        storage = FileKeyValueStore(
+          path: 'path',
+          fileName: 'file',
+          fs: fileSystem,
+        );
+      });
 
-      // assert value is updated
-      final value2 = await storage.readKey(key: 'key');
-      expect(value2, 'value');
-    });
+      tearDown(() async {
+        await storage.file.delete();
+      });
 
-    test('removeKey', () async {
-      // seed storage & assert value is present
-      await storage.writeKey(key: 'key', value: 'value');
-      final value1 = await storage.readKey(key: 'key');
-      expect(value1, 'value');
+      test('readKey & writeKey', () async {
+        // assert initial state is null
+        final value1 = await storage.readKey(key: 'key');
+        expect(value1, isNull);
 
-      // remove key
-      await storage.removeKey(key: 'key');
+        // write key-value pair
+        await storage.writeKey(key: 'key', value: 'value');
 
-      // assert key is removed
-      final value2 = await storage.readKey(key: 'key');
-      expect(value2, isNull);
-    });
+        // assert value is updated
+        final value2 = await storage.readKey(key: 'key');
+        expect(value2, 'value');
+      });
 
-    test('readAll', () async {
-      // write key-value pairs
-      await storage.writeKey(key: 'key1', value: 'value1');
-      await storage.writeKey(key: 'key2', value: 'value2');
+      test('removeKey', () async {
+        // seed storage & assert value is present
+        await storage.writeKey(key: 'key', value: 'value');
+        final value1 = await storage.readKey(key: 'key');
+        expect(value1, 'value');
 
-      // assert values are updated
-      final data = await storage.readAll();
-      expect(data['key1'], 'value1');
-      expect(data['key2'], 'value2');
-    });
+        // remove key
+        await storage.removeKey(key: 'key');
 
-    test('writeAll', () async {
-      // write key-value pairs
-      await storage.writeAll({
-        'key1': 'value1',
-        'key2': 'value2',
+        // assert key is removed
+        final value2 = await storage.readKey(key: 'key');
+        expect(value2, isNull);
       });
 
-      // assert values are updated
-      final data = await storage.readAll();
-      expect(data['key1'], 'value1');
-      expect(data['key2'], 'value2');
-    });
+      test('readAll', () async {
+        // write key-value pairs
+        await storage.writeKey(key: 'key1', value: 'value1');
+        await storage.writeKey(key: 'key2', value: 'value2');
 
-    test('includes', () async {
-      // write key-value pair
-      await storage.writeKey(key: 'key1', value: 'value1');
+        // assert values are updated
+        final data = await storage.readAll();
+        expect(data['key1'], 'value1');
+        expect(data['key2'], 'value2');
+      });
+
+      test('writeAll', () async {
+        // write key-value pairs
+        await storage.writeAll({
+          'key1': 'value1',
+          'key2': 'value2',
+        });
+
+        // assert values are updated
+        final data = await storage.readAll();
+        expect(data['key1'], 'value1');
+        expect(data['key2'], 'value2');
+      });
 
-      // assert that existing key returns true
-      final includesKey1 = await storage.containsKey(key: 'key1');
-      expect(includesKey1, isTrue);
+      test('includes', () async {
+        // write key-value pair
+        await storage.writeKey(key: 'key1', value: 'value1');
 
-      // assert that a non existing key returns false
-      final includesKey2 = await storage.containsKey(key: 'key2');
-      expect(includesKey2, isFalse);
+        // assert that existing key returns true
+        final includesKey1 = await storage.containsKey(key: 'key1');
+        expect(includesKey1, isTrue);
+
+        // assert that a non existing key returns false
+        final includesKey2 = await storage.containsKey(key: 'key2');
+        expect(includesKey2, isFalse);
+      });
+
+      group('parallel operations', () {
+        final items = List.generate(1000, ((i) => i));
+
+        test('should occur in the order they are called', () async {
+          final futures = items.map(
+            (i) async => storage.writeKey(key: 'key', value: i),
+          );
+          await Future.wait(futures);
+          final value = await storage.readKey(key: 'key');
+          expect(value, items.last);
+        });
+
+        test('should not result in stale data written to the file', () async {
+          final futures = items.map(
+            (i) async => storage.writeKey(key: 'key_$i', value: i),
+          );
+          await Future.wait(futures);
+          for (final i in items) {
+            final value = await storage.readKey(key: 'key_$i');
+            expect(value, items[i]);
+          }
+        });
+
+        // Reference: https://github.com/aws-amplify/amplify-flutter/issues/5190
+        test('should not corrupt the file', () async {
+          final futures = items.map(
+            (i) async {
+              if (i % 5 == 1) {
+                await storage.removeKey(key: 'key_${i - 1}');
+              }
+              return storage.writeKey(key: 'key_$i', value: 'value_$i');
+            },
+          );
+          await Future.wait(futures);
+        });
+      });
+
+      test('File is cleared when corrupted and can be re-written to', () async {
+        await storage.writeKey(key: 'foo', value: 'value');
+        final value1 = await storage.readKey(key: 'foo');
+        expect(value1, 'value');
+        await storage.file
+            .writeAsString('{invalid json}', mode: FileMode.append);
+        final value2 = await storage.readKey(key: 'foo');
+        expect(value2, null);
+        await storage.writeKey(key: 'foo', value: 'value');
+        final value3 = await storage.readKey(key: 'foo');
+        expect(value3, 'value');
+      });
     });
-  });
+  }
 }