Make MemoryRandomAccessFile handle the file being moved from under it…

… (dart-lang/file#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.

pkgs/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.

pkgs/file/lib/src/backends/memory/memory_file.dart

@@ -181,7 +181,7 @@ class MemoryFile extends MemoryFileSystemEntity implements File {
       createSync();
     }
 
-    return MemoryRandomAccessFile(this, resolvedBacking as FileNode, mode);
+    return MemoryRandomAccessFile(path, resolvedBacking as FileNode, mode);
   }
 
   @override
@@ -314,12 +314,28 @@ class _FileSink implements io.IOSink {
     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) {

pkgs/file/lib/src/backends/memory/memory_random_access_file.dart

@@ -18,7 +18,7 @@ class MemoryRandomAccessFile implements io.RandomAccessFile {
   /// 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 @@ class MemoryRandomAccessFile implements io.RandomAccessFile {
     }
   }
 
-  final MemoryFile _memoryFile;
+  @override
+  final String path;
+
   final FileNode _node;
   final io.FileMode _mode;
 
@@ -136,9 +138,6 @@ class MemoryRandomAccessFile implements io.RandomAccessFile {
     }
   }
 
-  @override
-  String get path => _memoryFile.path;
-
   @override
   Future<void> close() async => _asyncWrapper(closeSync);
 
@@ -167,7 +166,7 @@ class MemoryRandomAccessFile implements io.RandomAccessFile {
   int lengthSync() {
     _checkOpen();
     _checkAsync();
-    return _memoryFile.lengthSync();
+    return _node.size;
   }
 
   @override

pkgs/file/lib/src/backends/record_replay/common.dart

@@ -171,7 +171,8 @@ String describeInvocation(Invocation invocation) {
       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) {

pkgs/file/test/chroot_test.dart

@@ -22,8 +22,15 @@ void main() {
       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 @@ void main() {
 
           // https://github.com/dart-lang/sdk/issues/28277
           'Link > rename > throwsIfDestinationExistsAsFile',
+
+          ...skipCommon,
         ],
       );
     }, skip: io.Platform.isWindows);

pkgs/file/test/common_tests.dart

@@ -1273,10 +1273,11 @@ void runCommonTests(
         });
 
         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 @@ void runCommonTests(
                   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 @@ void runCommonTests(
                       '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 @@ void runCommonTests(
           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 @@ void runCommonTests(
           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 @@ void runCommonTests(
         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 @@ void runCommonTests(
 
         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 @@ void runCommonTests(
         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 @@ void runCommonTests(
         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 @@ void runCommonTests(
 
         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);
         });

pkgs/file/test/local_test.dart

@@ -91,6 +91,10 @@ void main() {
         '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',
       ],
     };
 

pkgs/file/test/replay_test.dart

@@ -212,22 +212,25 @@ void main() {
   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 @@ void main() {
       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)',
       );