Skip to content

Commit 26b970f

Browse files
Jordan-NelsonJordan-Nelson
authored
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
1 parent d62d543 commit 26b970f

File tree

2 files changed

+209
-87
lines changed

2 files changed

+209
-87
lines changed

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

Lines changed: 89 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
22
// SPDX-License-Identifier: Apache-2.0
33

4+
import 'dart:async';
45
import 'dart:convert';
56
import 'dart:io';
67

8+
import 'package:aws_common/aws_common.dart';
79
import 'package:file/file.dart' as pkg_file;
810
import 'package:file/local.dart' as local_file;
911
import 'package:meta/meta.dart';
@@ -18,7 +20,7 @@ import 'package:path/path.dart' as pkg_path;
1820
// without bringing in flutter as a dependency to the tests.
1921
class FileKeyValueStore {
2022
/// {@macro amplify_secure_storage_dart.file_key_value_store}
21-
const FileKeyValueStore({
23+
FileKeyValueStore({
2224
required this.path,
2325
required this.fileName,
2426
this.fs = const local_file.LocalFileSystem(),
@@ -32,27 +34,23 @@ class FileKeyValueStore {
3234
/// The file will be created if it does not yet exist.
3335
final String fileName;
3436

37+
final TaskScheduler _scheduler = TaskScheduler();
38+
39+
final AWSLogger logger = AWSLogger('FileKeyValueStore');
40+
3541
@visibleForTesting
3642
final pkg_file.FileSystem fs;
3743

44+
@visibleForTesting
3845
File get file => fs.file(
3946
pkg_path.join(
4047
path,
4148
fileName,
4249
),
4350
);
4451

45-
/// Writes a single key to storage.
46-
Future<void> writeKey({
47-
required String key,
48-
required Object value,
49-
}) async {
50-
final data = await readAll();
51-
data[key] = value;
52-
return writeAll(data);
53-
}
54-
55-
/// Overwrites the existing data.
52+
/// Overwrites the existing data in [file] with the key-value pairs in [data].
53+
@visibleForTesting
5654
Future<void> writeAll(
5755
Map<String, Object> data,
5856
) async {
@@ -63,42 +61,104 @@ class FileKeyValueStore {
6361
await file.writeAsString(stringMap);
6462
}
6563

64+
/// Reads all the key-value pairs from [file].
65+
@visibleForTesting
66+
Future<Map<String, Object>> readAll() async {
67+
if (await file.exists()) {
68+
final stringMap = await file.readAsString();
69+
if (stringMap.isNotEmpty) {
70+
try {
71+
final Object? data = json.decode(stringMap);
72+
if (data is Map) {
73+
return data.cast<String, Object>();
74+
}
75+
} on FormatException catch (e) {
76+
logger.error(
77+
'Cannot read file. The file may be corrupted. '
78+
'Clearing file contents. See error for more details. '
79+
'Error: $e',
80+
);
81+
await writeAll({});
82+
}
83+
}
84+
}
85+
return <String, Object>{};
86+
}
87+
88+
/// Writes a single key to storage.
89+
Future<void> writeKey({
90+
required String key,
91+
required Object value,
92+
}) async {
93+
return _scheduler.schedule(() async {
94+
final data = await readAll();
95+
data[key] = value;
96+
return writeAll(data);
97+
});
98+
}
99+
66100
/// Reads a single key from storage.
67101
Future<Object?> readKey({
68102
required String key,
69103
}) async {
70-
final data = await readAll();
71-
return data[key];
104+
return _scheduler.schedule(() async {
105+
final data = await readAll();
106+
return data[key];
107+
});
72108
}
73109

74110
/// Removes a single key from storage.
75111
Future<void> removeKey({
76112
required String key,
77113
}) async {
78-
final data = await readAll();
79-
data.remove(key);
80-
await writeAll(data);
114+
return _scheduler.schedule(() async {
115+
final data = await readAll();
116+
data.remove(key);
117+
await writeAll(data);
118+
});
81119
}
82120

83121
/// Returns true if the key exists in storage
84122
Future<bool> containsKey({
85123
required String key,
86124
}) async {
87-
final data = await readAll();
88-
return data.containsKey(key);
125+
return _scheduler.schedule(() async {
126+
final data = await readAll();
127+
return data.containsKey(key);
128+
});
129+
}
130+
}
131+
132+
/// A class for processing async tasks one at a time in the order that they are
133+
/// scheduled.
134+
class TaskScheduler {
135+
final _taskQueue = <Task<dynamic>>[];
136+
bool _isProcessing = false;
137+
Future<T> schedule<T>(Future<T> Function() task) {
138+
final completer = Completer<T>();
139+
_taskQueue.add(Task(task, completer));
140+
_processTasks();
141+
return completer.future;
89142
}
90143

91-
/// Reads all the key-value pairs from storage.
92-
Future<Map<String, Object>> readAll() async {
93-
if (await file.exists()) {
94-
final stringMap = await file.readAsString();
95-
if (stringMap.isNotEmpty) {
96-
final Object? data = json.decode(stringMap);
97-
if (data is Map) {
98-
return data.cast<String, Object>();
99-
}
144+
Future<void> _processTasks() async {
145+
if (_isProcessing) return;
146+
_isProcessing = true;
147+
while (_taskQueue.isNotEmpty) {
148+
final currentTask = _taskQueue.removeAt(0);
149+
try {
150+
final result = await currentTask.task();
151+
currentTask.completer.complete(result);
152+
} on Object catch (e, stackTrace) {
153+
currentTask.completer.completeError(e, stackTrace);
100154
}
101155
}
102-
return <String, Object>{};
156+
_isProcessing = false;
103157
}
104158
}
159+
160+
class Task<T> {
161+
Task(this.task, this.completer);
162+
final Future<T> Function() task;
163+
final Completer<T> completer;
164+
}

‎packages/secure_storage/amplify_secure_storage_test/test/file_key_value_store_test.dart

Lines changed: 120 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -3,83 +3,145 @@
33

44
@TestOn('vm')
55

6+
import 'dart:io';
7+
68
import 'package:amplify_secure_storage_dart/src/utils/file_key_value_store.dart';
9+
import 'package:file/local.dart';
710
import 'package:file/memory.dart';
811
import 'package:test/test.dart';
912

1013
void main() {
1114
late FileKeyValueStore storage;
12-
group('FileKeyValueStore', () {
13-
setUp(() {
14-
storage = FileKeyValueStore(
15-
path: 'path',
16-
fileName: 'file',
17-
fs: MemoryFileSystem(),
18-
);
19-
});
2015

21-
test('readKey & writeKey', () async {
22-
// assert initial state is null
23-
final value1 = await storage.readKey(key: 'key');
24-
expect(value1, isNull);
16+
final fileSystems = [MemoryFileSystem(), const LocalFileSystem()];
2517

26-
// write key-value pair
27-
await storage.writeKey(key: 'key', value: 'value');
18+
for (final fileSystem in fileSystems) {
19+
group('FileKeyValueStore (${fileSystem.runtimeType})', () {
20+
setUp(() {
21+
storage = FileKeyValueStore(
22+
path: 'path',
23+
fileName: 'file',
24+
fs: fileSystem,
25+
);
26+
});
2827

29-
// assert value is updated
30-
final value2 = await storage.readKey(key: 'key');
31-
expect(value2, 'value');
32-
});
28+
tearDown(() async {
29+
await storage.file.delete();
30+
});
3331

34-
test('removeKey', () async {
35-
// seed storage & assert value is present
36-
await storage.writeKey(key: 'key', value: 'value');
37-
final value1 = await storage.readKey(key: 'key');
38-
expect(value1, 'value');
32+
test('readKey & writeKey', () async {
33+
// assert initial state is null
34+
final value1 = await storage.readKey(key: 'key');
35+
expect(value1, isNull);
3936

40-
// remove key
41-
await storage.removeKey(key: 'key');
37+
// write key-value pair
38+
await storage.writeKey(key: 'key', value: 'value');
4239

43-
// assert key is removed
44-
final value2 = await storage.readKey(key: 'key');
45-
expect(value2, isNull);
46-
});
40+
// assert value is updated
41+
final value2 = await storage.readKey(key: 'key');
42+
expect(value2, 'value');
43+
});
4744

48-
test('readAll', () async {
49-
// write key-value pairs
50-
await storage.writeKey(key: 'key1', value: 'value1');
51-
await storage.writeKey(key: 'key2', value: 'value2');
45+
test('removeKey', () async {
46+
// seed storage & assert value is present
47+
await storage.writeKey(key: 'key', value: 'value');
48+
final value1 = await storage.readKey(key: 'key');
49+
expect(value1, 'value');
5250

53-
// assert values are updated
54-
final data = await storage.readAll();
55-
expect(data['key1'], 'value1');
56-
expect(data['key2'], 'value2');
57-
});
51+
// remove key
52+
await storage.removeKey(key: 'key');
5853

59-
test('writeAll', () async {
60-
// write key-value pairs
61-
await storage.writeAll({
62-
'key1': 'value1',
63-
'key2': 'value2',
54+
// assert key is removed
55+
final value2 = await storage.readKey(key: 'key');
56+
expect(value2, isNull);
6457
});
6558

66-
// assert values are updated
67-
final data = await storage.readAll();
68-
expect(data['key1'], 'value1');
69-
expect(data['key2'], 'value2');
70-
});
59+
test('readAll', () async {
60+
// write key-value pairs
61+
await storage.writeKey(key: 'key1', value: 'value1');
62+
await storage.writeKey(key: 'key2', value: 'value2');
7163

72-
test('includes', () async {
73-
// write key-value pair
74-
await storage.writeKey(key: 'key1', value: 'value1');
64+
// assert values are updated
65+
final data = await storage.readAll();
66+
expect(data['key1'], 'value1');
67+
expect(data['key2'], 'value2');
68+
});
69+
70+
test('writeAll', () async {
71+
// write key-value pairs
72+
await storage.writeAll({
73+
'key1': 'value1',
74+
'key2': 'value2',
75+
});
76+
77+
// assert values are updated
78+
final data = await storage.readAll();
79+
expect(data['key1'], 'value1');
80+
expect(data['key2'], 'value2');
81+
});
7582

76-
// assert that existing key returns true
77-
final includesKey1 = await storage.containsKey(key: 'key1');
78-
expect(includesKey1, isTrue);
83+
test('includes', () async {
84+
// write key-value pair
85+
await storage.writeKey(key: 'key1', value: 'value1');
7986

80-
// assert that a non existing key returns false
81-
final includesKey2 = await storage.containsKey(key: 'key2');
82-
expect(includesKey2, isFalse);
87+
// assert that existing key returns true
88+
final includesKey1 = await storage.containsKey(key: 'key1');
89+
expect(includesKey1, isTrue);
90+
91+
// assert that a non existing key returns false
92+
final includesKey2 = await storage.containsKey(key: 'key2');
93+
expect(includesKey2, isFalse);
94+
});
95+
96+
group('parallel operations', () {
97+
final items = List.generate(1000, ((i) => i));
98+
99+
test('should occur in the order they are called', () async {
100+
final futures = items.map(
101+
(i) async => storage.writeKey(key: 'key', value: i),
102+
);
103+
await Future.wait(futures);
104+
final value = await storage.readKey(key: 'key');
105+
expect(value, items.last);
106+
});
107+
108+
test('should not result in stale data written to the file', () async {
109+
final futures = items.map(
110+
(i) async => storage.writeKey(key: 'key_$i', value: i),
111+
);
112+
await Future.wait(futures);
113+
for (final i in items) {
114+
final value = await storage.readKey(key: 'key_$i');
115+
expect(value, items[i]);
116+
}
117+
});
118+
119+
// Reference: https://github.com/aws-amplify/amplify-flutter/issues/5190
120+
test('should not corrupt the file', () async {
121+
final futures = items.map(
122+
(i) async {
123+
if (i % 5 == 1) {
124+
await storage.removeKey(key: 'key_${i - 1}');
125+
}
126+
return storage.writeKey(key: 'key_$i', value: 'value_$i');
127+
},
128+
);
129+
await Future.wait(futures);
130+
});
131+
});
132+
133+
test('File is cleared when corrupted and can be re-written to', () async {
134+
await storage.writeKey(key: 'foo', value: 'value');
135+
final value1 = await storage.readKey(key: 'foo');
136+
expect(value1, 'value');
137+
await storage.file
138+
.writeAsString('{invalid json}', mode: FileMode.append);
139+
final value2 = await storage.readKey(key: 'foo');
140+
expect(value2, null);
141+
await storage.writeKey(key: 'foo', value: 'value');
142+
final value3 = await storage.readKey(key: 'foo');
143+
expect(value3, 'value');
144+
});
83145
});
84-
});
146+
}
85147
}

0 commit comments

Comments
 (0)