Make MemoryRandomAccessFile handle the file being moved from under it (#157)
* Make rename tests verify the path of the returned FileSystemEntity
* Add explicit types to fix analysis lints
* Make MemoryRandomAccessFile handle the file being moved from under it
Fix `MemoryRandomAccessFile` to use only its `FileNode` and never the
corresponding `MemoryFile`. While the `MemoryRandomAccessFile` (the
equivalent of a file handle) is open, its `FileNode` should never
change, but the original `MemoryFile` might become backed by a
different node if the file is moved, removed, or replaced.
POSIX systems typically allow this; Windows typically doesn't. For
now, make `MemoryFileSystem` follow the typical POSIX behavior. (In
the long-term, `MemoryFileSystem` perhaps should have configurable
behavior.)
* Make MemoryFile.openWrite better handle a file being moved from under it
Make `MemoryFile.openWrite`'s `IOSink` resolve the `FileNode`
immediately instead of adding it to an async queue. Otherwise node
resolution could return a different `FileNode` than what was
originally intended.
Eagerly resolving the `FileNode` has the side-effect of potentially
throwing exceptions earlier than expected. I therefore added a
mechanism to defer throwing.
diff --git a/packages/file/CHANGELOG.md b/packages/file/CHANGELOG.md
index a30c55b..22c292c 100644
--- a/packages/file/CHANGELOG.md
+++ b/packages/file/CHANGELOG.md
@@ -1,4 +1,7 @@
#### 5.2.2-dev
+
+* Made `MemoryRandomAccessFile` and `MemoryFile.openWrite` handle the file
+ being removed or renamed while open.
* Fixed incorrect formatting in `NoMatchingInvocationError.toString()`.
* Fixed more test flakiness.
* Enabled more tests.
diff --git a/packages/file/lib/src/backends/memory/memory_file.dart b/packages/file/lib/src/backends/memory/memory_file.dart
index 8df0a0c..bda9ae9 100644
--- a/packages/file/lib/src/backends/memory/memory_file.dart
+++ b/packages/file/lib/src/backends/memory/memory_file.dart
@@ -181,7 +181,7 @@
createSync();
}
- return MemoryRandomAccessFile(this, resolvedBacking as FileNode, mode);
+ return MemoryRandomAccessFile(path, resolvedBacking as FileNode, mode);
}
@override
@@ -314,12 +314,28 @@
io.FileMode mode,
Encoding encoding,
) {
- Future<FileNode> node = Future<FileNode>.microtask(() {
- FileNode node = file._resolvedBackingOrCreate;
+ FileNode node;
+ Exception deferredException;
+
+ // Resolve the backing immediately to ensure that the [FileNode] we write
+ // to is the same as when [openWrite] was called. This can matter if the
+ // file is moved or removed while open.
+ try {
+ node = file._resolvedBackingOrCreate;
+ } on Exception catch (e) {
+ // For behavioral consistency with [LocalFile], do not report failures
+ // immediately.
+ deferredException = e;
+ }
+
+ Future<FileNode> future = Future<FileNode>.microtask(() {
+ if (deferredException != null) {
+ throw deferredException;
+ }
file._truncateIfNecessary(node, mode);
return node;
});
- return _FileSink._(node, encoding);
+ return _FileSink._(future, encoding);
}
_FileSink._(this._node, this.encoding) {
diff --git a/packages/file/lib/src/backends/memory/memory_random_access_file.dart b/packages/file/lib/src/backends/memory/memory_random_access_file.dart
index 568195b..b30389f 100644
--- a/packages/file/lib/src/backends/memory/memory_random_access_file.dart
+++ b/packages/file/lib/src/backends/memory/memory_random_access_file.dart
@@ -18,7 +18,7 @@
/// Constructs a [MemoryRandomAccessFile].
///
/// This should be used only by [MemoryFile.open] or [MemoryFile.openSync].
- MemoryRandomAccessFile(this._memoryFile, this._node, this._mode) {
+ MemoryRandomAccessFile(this.path, this._node, this._mode) {
switch (_mode) {
case io.FileMode.read:
break;
@@ -36,7 +36,9 @@
}
}
- final MemoryFile _memoryFile;
+ @override
+ final String path;
+
final FileNode _node;
final io.FileMode _mode;
@@ -137,9 +139,6 @@
}
@override
- String get path => _memoryFile.path;
-
- @override
Future<void> close() async => _asyncWrapper(closeSync);
@override
@@ -167,7 +166,7 @@
int lengthSync() {
_checkOpen();
_checkAsync();
- return _memoryFile.lengthSync();
+ return _node.size;
}
@override
diff --git a/packages/file/lib/src/backends/record_replay/common.dart b/packages/file/lib/src/backends/record_replay/common.dart
index ce5b2ef..be610bf 100644
--- a/packages/file/lib/src/backends/record_replay/common.dart
+++ b/packages/file/lib/src/backends/record_replay/common.dart
@@ -171,7 +171,8 @@
buffer.write(Error.safeToString(encode(arg)));
printedCount += 1;
}
- for (final nameValue in invocation.namedArguments.entries) {
+ for (final MapEntry<Symbol, dynamic> nameValue
+ in invocation.namedArguments.entries) {
final Symbol name = nameValue.key;
final dynamic value = nameValue.value;
if (printedCount > 0) {
diff --git a/packages/file/test/chroot_test.dart b/packages/file/test/chroot_test.dart
index a320511..997411c 100644
--- a/packages/file/test/chroot_test.dart
+++ b/packages/file/test/chroot_test.dart
@@ -22,8 +22,15 @@
return ChrootFileSystem(fs, '/tmp');
}
+ // TODO(jamesderlin): Make ChrootFile.openSync return a delegating
+ // RandomAccessFile that uses the chroot'd path.
+ List<String> skipCommon = <String>[
+ 'File > open > .* > RandomAccessFile > read > openReadHandleDoesNotChange',
+ 'File > open > .* > RandomAccessFile > openWriteHandleDoesNotChange',
+ ];
+
group('memoryBacked', () {
- runCommonTests(createMemoryBackedChrootFileSystem);
+ runCommonTests(createMemoryBackedChrootFileSystem, skip: skipCommon);
});
group('localBacked', () {
@@ -48,6 +55,8 @@
// https://github.com/dart-lang/sdk/issues/28277
'Link > rename > throwsIfDestinationExistsAsFile',
+
+ ...skipCommon,
],
);
}, skip: io.Platform.isWindows);
diff --git a/packages/file/test/common_tests.dart b/packages/file/test/common_tests.dart
index 3b875d4..702a441 100644
--- a/packages/file/test/common_tests.dart
+++ b/packages/file/test/common_tests.dart
@@ -1273,10 +1273,11 @@
});
test('succeedsIfDestinationDoesntExistAtTail', () {
- File f = fs.file(ns('/foo'))..createSync();
- f.renameSync(ns('/bar'));
+ File src = fs.file(ns('/foo'))..createSync();
+ File dest = src.renameSync(ns('/bar'));
expect(fs.file(ns('/foo')), isNot(exists));
expect(fs.file(ns('/bar')), exists);
+ expect(dest.path, ns('/bar'));
});
test('throwsIfDestinationDoesntExistViaTraversal', () {
@@ -1855,6 +1856,21 @@
expect(numRead, 3);
expect(utf8.decode(buffer.sublist(2, 5)), 'pre');
});
+
+ test('openReadHandleDoesNotChange', () {
+ final String initial = utf8.decode(raf.readSync(4));
+ expect(initial, 'pre-');
+ final File newFile = f.renameSync(ns('/bar'));
+ String rest = utf8.decode(raf.readSync(1024));
+ expect(rest, 'existing content\n');
+
+ assert(newFile.path != f.path);
+ expect(f, isNot(exists));
+ expect(newFile, exists);
+
+ // [RandomAccessFile.path] always returns the original path.
+ expect(raf.path, f.path);
+ });
});
}
@@ -1942,6 +1958,26 @@
'pre-existing content\nHello world');
}
});
+
+ test('openWriteHandleDoesNotChange', () {
+ raf.writeStringSync('Hello ');
+ final File newFile = f.renameSync(ns('/bar'));
+ raf.writeStringSync('world');
+
+ final String contents = newFile.readAsStringSync();
+ if (mode == FileMode.write || mode == FileMode.writeOnly) {
+ expect(contents, 'Hello world');
+ } else {
+ expect(contents, 'pre-existing content\nHello world');
+ }
+
+ assert(newFile.path != f.path);
+ expect(f, isNot(exists));
+ expect(newFile, exists);
+
+ // [RandomAccessFile.path] always returns the original path.
+ expect(raf.path, f.path);
+ });
}
if (mode == FileMode.append || mode == FileMode.writeOnlyAppend) {
@@ -2147,6 +2183,48 @@
expect(stream.isBroadcast, isFalse);
await stream.drain<void>();
});
+
+ test('openReadHandleDoesNotChange', () async {
+ // Ideally, `data` should be large enough so that its contents are
+ // split across multiple chunks in the [Stream]. However, there
+ // doesn't seem to be a good way to determine the chunk size used by
+ // [io.File].
+ final List<int> data = List<int>.generate(
+ 1024 * 256,
+ (int index) => index & 0xFF,
+ growable: false,
+ );
+
+ final File f = fs.file(ns('/foo'))..createSync();
+
+ f.writeAsBytesSync(data, flush: true);
+ final Stream<List<int>> stream = f.openRead();
+
+ File newFile;
+ List<int> initialChunk;
+ final List<int> remainingChunks = <int>[];
+
+ await for (List<int> chunk in stream) {
+ if (initialChunk == null) {
+ initialChunk = chunk;
+ assert(initialChunk.isNotEmpty);
+ expect(initialChunk, data.getRange(0, initialChunk.length));
+
+ newFile = f.renameSync(ns('/bar'));
+ } else {
+ remainingChunks.addAll(chunk);
+ }
+ }
+
+ expect(
+ remainingChunks,
+ data.getRange(initialChunk.length, data.length),
+ );
+
+ assert(newFile.path != f.path);
+ expect(f, isNot(exists));
+ expect(newFile, exists);
+ });
});
group('openWrite', () {
@@ -2212,6 +2290,24 @@
expect(fs.file(ns('/foo')).readAsStringSync(), 'HelloGoodbye');
});
+ test('openWriteHandleDoesNotChange', () async {
+ File f = fs.file(ns('/foo'))..createSync();
+ IOSink sink = f.openWrite();
+ sink.write('Hello');
+ await sink.flush();
+
+ final File newFile = f.renameSync(ns('/bar'));
+ sink.write('Goodbye');
+ await sink.flush();
+ await sink.close();
+
+ expect(newFile.readAsStringSync(), 'HelloGoodbye');
+
+ assert(newFile.path != f.path);
+ expect(f, isNot(exists));
+ expect(newFile, exists);
+ });
+
group('ioSink', () {
File f;
IOSink sink;
@@ -3254,7 +3350,8 @@
test('succeedsIfSourceIsLinkToFile', () {
Link l = fs.link(ns('/foo'))..createSync(ns('/bar'));
fs.file(ns('/bar')).createSync();
- l.renameSync(ns('/baz'));
+ Link renamed = l.renameSync(ns('/baz'));
+ expect(renamed.path, ns('/baz'));
expect(fs.typeSync(ns('/foo'), followLinks: false),
FileSystemEntityType.notFound);
expect(fs.typeSync(ns('/bar'), followLinks: false),
@@ -3266,7 +3363,8 @@
test('succeedsIfSourceIsLinkToNotFound', () {
Link l = fs.link(ns('/foo'))..createSync(ns('/bar'));
- l.renameSync(ns('/baz'));
+ Link renamed = l.renameSync(ns('/baz'));
+ expect(renamed.path, ns('/baz'));
expect(fs.typeSync(ns('/foo'), followLinks: false),
FileSystemEntityType.notFound);
expect(fs.typeSync(ns('/baz'), followLinks: false),
@@ -3277,7 +3375,8 @@
test('succeedsIfSourceIsLinkToDirectory', () {
Link l = fs.link(ns('/foo'))..createSync(ns('/bar'));
fs.directory(ns('/bar')).createSync();
- l.renameSync(ns('/baz'));
+ Link renamed = l.renameSync(ns('/baz'));
+ expect(renamed.path, ns('/baz'));
expect(fs.typeSync(ns('/foo'), followLinks: false),
FileSystemEntityType.notFound);
expect(fs.typeSync(ns('/bar'), followLinks: false),
@@ -3290,7 +3389,8 @@
test('succeedsIfSourceIsLinkLoop', () {
Link l = fs.link(ns('/foo'))..createSync(ns('/bar'));
fs.link(ns('/bar')).createSync(ns('/foo'));
- l.renameSync(ns('/baz'));
+ Link renamed = l.renameSync(ns('/baz'));
+ expect(renamed.path, ns('/baz'));
expect(fs.typeSync(ns('/foo'), followLinks: false),
FileSystemEntityType.notFound);
expect(fs.typeSync(ns('/bar'), followLinks: false),
@@ -3302,7 +3402,8 @@
test('succeedsIfDestinationDoesntExistAtTail', () {
Link l = fs.link(ns('/foo'))..createSync(ns('/bar'));
- l.renameSync(ns('/baz'));
+ Link renamed = l.renameSync(ns('/baz'));
+ expect(renamed.path, ns('/baz'));
expect(fs.link(ns('/foo')), isNot(exists));
expect(fs.link(ns('/baz')), exists);
});
diff --git a/packages/file/test/local_test.dart b/packages/file/test/local_test.dart
index bd1da4a..ef2ffdd 100644
--- a/packages/file/test/local_test.dart
+++ b/packages/file/test/local_test.dart
@@ -91,6 +91,10 @@
'File > open > APPEND > RandomAccessFile > truncate > throwsIfSetToNegativeNumber',
'File > open > WRITE_ONLY > RandomAccessFile > truncate > throwsIfSetToNegativeNumber',
'File > open > WRITE_ONLY_APPEND > RandomAccessFile > truncate > throwsIfSetToNegativeNumber',
+
+ // Windows does not allow removing or renaming open files.
+ '.* > openReadHandleDoesNotChange',
+ '.* > openWriteHandleDoesNotChange',
],
};
diff --git a/packages/file/test/replay_test.dart b/packages/file/test/replay_test.dart
index 36edbd6..192d161 100644
--- a/packages/file/test/replay_test.dart
+++ b/packages/file/test/replay_test.dart
@@ -212,22 +212,25 @@
group('describeInvocation', () {
test('methodWithNoArguments', () {
expect(
- describeInvocation(Invocation.method(#foo, [])),
+ describeInvocation(Invocation.method(#foo, <Object>[])),
'foo()',
);
});
test('methodWithOnlyPositionalArguments', () {
expect(
- describeInvocation(Invocation.method(#foo, [1, 'bar', null])),
+ describeInvocation(Invocation.method(#foo, <Object>[1, 'bar', null])),
'foo(1, "bar", null)',
);
});
test('methodWithOnlyNamedArguments', () {
expect(
- describeInvocation(
- Invocation.method(#foo, [], {#x: 2, #y: 'baz', #z: null})),
+ describeInvocation(Invocation.method(
+ #foo,
+ <Object>[],
+ <Symbol, Object>{#x: 2, #y: 'baz', #z: null},
+ )),
'foo(x: 2, y: "baz", z: null)',
);
});
@@ -236,8 +239,8 @@
expect(
describeInvocation(Invocation.method(
#foo,
- [1, 'bar', null],
- {#x: 2, #y: 'baz', #z: null},
+ <Object>[1, 'bar', null],
+ <Symbol, Object>{#x: 2, #y: 'baz', #z: null},
)),
'foo(1, "bar", null, x: 2, y: "baz", z: null)',
);
