-
Notifications
You must be signed in to change notification settings - Fork 266
fix: file stream sharing #1363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: file stream sharing #1363
Changes from all commits
2739e85
ec701cf
2a70ae1
18716e4
c660094
d648b5e
9952eab
4e724f5
c9e04a2
f241cc2
eabc64e
2db45c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | |||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,6 +2,7 @@ | ||||||||||||||||||||||||||||||||||||||||
| using System.Threading; | |||||||||||||||||||||||||||||||||||||||||
| using System.Runtime.Versioning; | |||||||||||||||||||||||||||||||||||||||||
| using System.Security.AccessControl; | |||||||||||||||||||||||||||||||||||||||||
| using System.Collections.Concurrent; | |||||||||||||||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||||||||||||||
| namespace System.IO.Abstractions.TestingHelpers; | |||||||||||||||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||||||||||||||
|
|
@@ -32,16 +33,19 @@ public NullFileSystemStream() : base(Null, ".", true) | ||||||||||||||||||||||||||||||||||||||||
| private readonly IMockFileDataAccessor mockFileDataAccessor; | |||||||||||||||||||||||||||||||||||||||||
| private readonly string path; | |||||||||||||||||||||||||||||||||||||||||
| private readonly FileAccess access = FileAccess.ReadWrite; | |||||||||||||||||||||||||||||||||||||||||
| private readonly FileShare share = FileShare.Read; | |||||||||||||||||||||||||||||||||||||||||
| private readonly FileOptions options; | |||||||||||||||||||||||||||||||||||||||||
| private readonly MockFileData fileData; | |||||||||||||||||||||||||||||||||||||||||
| private bool disposed; | |||||||||||||||||||||||||||||||||||||||||
| private static ConcurrentDictionary<string, byte> _fileShareNoneStreams = []; | |||||||||||||||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||||||||||||||
| /// <inheritdoc /> | |||||||||||||||||||||||||||||||||||||||||
| public MockFileStream( | |||||||||||||||||||||||||||||||||||||||||
| IMockFileDataAccessor mockFileDataAccessor, | |||||||||||||||||||||||||||||||||||||||||
| string path, | |||||||||||||||||||||||||||||||||||||||||
| FileMode mode, | |||||||||||||||||||||||||||||||||||||||||
| FileAccess access = FileAccess.ReadWrite, | |||||||||||||||||||||||||||||||||||||||||
| FileShare share = FileShare.Read, | |||||||||||||||||||||||||||||||||||||||||
| FileOptions options = FileOptions.None) | |||||||||||||||||||||||||||||||||||||||||
| : base(new MemoryStream(), | |||||||||||||||||||||||||||||||||||||||||
| path == null ? null : Path.GetFullPath(path), | |||||||||||||||||||||||||||||||||||||||||
|
|
@@ -51,9 +55,15 @@ public MockFileStream( | ||||||||||||||||||||||||||||||||||||||||
| ThrowIfInvalidModeAccess(mode, access); | |||||||||||||||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||||||||||||||
| this.mockFileDataAccessor = mockFileDataAccessor ?? throw new ArgumentNullException(nameof(mockFileDataAccessor)); | |||||||||||||||||||||||||||||||||||||||||
| path = mockFileDataAccessor.PathVerifier.FixPath(path); | |||||||||||||||||||||||||||||||||||||||||
| this.path = path; | |||||||||||||||||||||||||||||||||||||||||
| this.options = options; | |||||||||||||||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||||||||||||||
| if (_fileShareNoneStreams.ContainsKey(path)) | |||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This now works only for
I think it would be a great idea to also handle these cases correctly. What do you think?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree. I briefly mentioned an approach to this in a previous comment, where instead of just using the This is the approach I am planning to go for and start working on, but let me know if you think there's a better idea I'm overlooking or something I'm missing!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I fear the solution is not so easy, as you can have multiple streams with e.g. read access and read share open at the same time and only when all streams got disposed should you be able to open the file with write access. So you would need to create a disposable access reference for each stream and remove this instance upon disposal of the stream from some kind of storage. For reference: in Testably.Abstractions I used a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yes, multiple streams of the same file open at once... |
|||||||||||||||||||||||||||||||||||||||||
| { | |||||||||||||||||||||||||||||||||||||||||
| throw CommonExceptions.ProcessCannotAccessFileInUse(path); | |||||||||||||||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||||||||||||||
| if (mockFileDataAccessor.FileExists(path)) | |||||||||||||||||||||||||||||||||||||||||
| { | |||||||||||||||||||||||||||||||||||||||||
| if (mode.Equals(FileMode.CreateNew)) | |||||||||||||||||||||||||||||||||||||||||
|
|
@@ -97,7 +107,15 @@ public MockFileStream( | ||||||||||||||||||||||||||||||||||||||||
| mockFileDataAccessor.AddFile(path, fileData); | |||||||||||||||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||||||||||||||
| if (share is FileShare.None) | |||||||||||||||||||||||||||||||||||||||||
| { | |||||||||||||||||||||||||||||||||||||||||
| if (!_fileShareNoneStreams.TryAdd(path, 0)) | |||||||||||||||||||||||||||||||||||||||||
| { | |||||||||||||||||||||||||||||||||||||||||
| throw CommonExceptions.ProcessCannotAccessFileInUse(path); | |||||||||||||||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||||||||||||||
| this.access = access; | |||||||||||||||||||||||||||||||||||||||||
| this.share = share; | |||||||||||||||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||||||||||||||
| private static void ThrowIfInvalidModeAccess(FileMode mode, FileAccess access) | |||||||||||||||||||||||||||||||||||||||||
|
|
@@ -144,6 +162,10 @@ protected override void Dispose(bool disposing) | ||||||||||||||||||||||||||||||||||||||||
| { | |||||||||||||||||||||||||||||||||||||||||
| return; | |||||||||||||||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||||||||||||||
| if (share is FileShare.None) | |||||||||||||||||||||||||||||||||||||||||
| { | |||||||||||||||||||||||||||||||||||||||||
| _fileShareNoneStreams.TryRemove(path, out _); | |||||||||||||||||||||||||||||||||||||||||
| } | |||||||||||||||||||||||||||||||||||||||||
| InternalFlush(); | |||||||||||||||||||||||||||||||||||||||||
| base.Dispose(disposing); | |||||||||||||||||||||||||||||||||||||||||
| OnClose(); | |||||||||||||||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -390,4 +390,26 @@ async Task Act() => | |
| await source.CopyToAsync(destination); | ||
| await That(Act).Throws<NotSupportedException>(); | ||
| } | ||
|
|
||
| [Test] | ||
| public async Task MockFileStream_WhenExclusiveStreamOpen_ShouldThrowIOException() | ||
| { | ||
| var fileSystem = new MockFileSystem(); | ||
| fileSystem.File.WriteAllText("foo.txt", ""); | ||
| using (new MockFileStream(fileSystem, "foo.txt", FileMode.Open, FileAccess.Read, FileShare.None)) | ||
| { | ||
| await That(() => new MockFileStream(fileSystem, "foo.txt", FileMode.Open, FileAccess.Read)).Throws<IOException>(); | ||
| } | ||
| } | ||
|
|
||
|
Comment on lines
+402
to
+404
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you could combine these two tests into one and verify that after you dispose of the stream it no longer throws... |
||
| [Test] | ||
| public async Task MockFileStream_WhenExclusiveStreamClosed_ShouldNotThrow() | ||
| { | ||
| var fileSystem = new MockFileSystem(); | ||
| fileSystem.File.WriteAllText("foo.txt", ""); | ||
| var stream = new MockFileStream(fileSystem, "foo.txt", FileMode.Open, FileAccess.Read, FileShare.None); | ||
| stream.Dispose(); | ||
|
|
||
| await That(() => new MockFileStream(fileSystem, "foo.txt", FileMode.Open, FileAccess.Read)).DoesNotThrow(); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to add some normalization before using the path as the dictionary key. Relative paths passed to the ctor could resolve to the same file system object. The
mockFileDataAccessordoes like this:System.IO.Abstractions/src/TestableIO.System.IO.Abstractions.TestingHelpers/MockFileStream.cs
Line 57 in 72ea54d
System.IO.Abstractions/src/TestableIO.System.IO.Abstractions.TestingHelpers/MockFileSystem.cs
Line 476 in 72ea54d
System.IO.Abstractions/src/TestableIO.System.IO.Abstractions.TestingHelpers/MockFileSystem.cs
Lines 131 to 142 in 72ea54d
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your comment and comprehensive snippets! This is a great point.
I added a private
NormalizePathmethod to theMockFileSystemclass largely based on the above, which should handle relative paths passed to the ctor pointing to the same object.I recognise there is a slight duplication of logic here, and maybe a more rigorous refactor with a general-use, shared
FixPathmethod somewhere (or potentially just makingFixPathpublic?), instead of having these two private methods, would be preferred - this could be worth some discussion.Please let me know your thoughts, and if you think the normalization implemented is sufficient!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to de-duplicate this logic into an internal method that can be used whenever we need to normalize a path to ensure it is done the same everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I have been digging around the codebase, and it seems to me a good approach to this would be to move the
FixPathmethod (and associated other methods) logic from theMockFileSystemclass into thePathVerifierclass, updating bothMockFileSystemandMockFileStreamaccordingly?As usual, its possible I'm missing something here or overlooking a more apt approach that those with more experience in this codebase may be able to see, so please do let me know if you think there's a better approach!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds reasonable...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be covered in eabc64e (explicit API test runs in following commit), let me know if there are any other desired changes here!