Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
+166 −35
Open

fix: file stream sharing #1363

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
2739e85
fix: added correct handling of file share in file stream constructor/…
HarrisonTCodes Oct 18, 2025
ec701cf
fix: added stateful tracking of unshared file streams and prevented m…
HarrisonTCodes Oct 18, 2025
2a70ae1
refactor: changed fileshare none streams state to use concurrent dict…
HarrisonTCodes Oct 30, 2025
18716e4
refactor: used existing common exception for file-in-use error in fil…
HarrisonTCodes Oct 30, 2025
c660094
feat: added handling of failed addition of exclusive file stream to t…
HarrisonTCodes Oct 30, 2025
d648b5e
chore: explicit API acceptance test changes
HarrisonTCodes Oct 31, 2025
9952eab
test: added exclusive mock file stream handling unit tests
HarrisonTCodes Oct 31, 2025
4e724f5
feat: added path normalization to mock file stream
HarrisonTCodes Oct 31, 2025
c9e04a2
fix: improved path normalization in mock file stream for relative paths
HarrisonTCodes Oct 31, 2025
f241cc2
fix: added improved handling of file stream options in factory method
HarrisonTCodes Nov 9, 2025
eabc64e
refactor: de-duplicated normalize/fix path logic moving method to pat…
HarrisonTCodes Nov 9, 2025
2db45c1
chore: explicit API acceptance test changes to cover path verifier ch…
HarrisonTCodes Nov 9, 2025
611907b
feat: added more rigorous tracking of open file streams and shares/ac…
HarrisonTCodes Nov 13, 2025
cd2a151
feat: moved open file handles state to mock file system and ran API a…
HarrisonTCodes Nov 14, 2025
fd3062a
feat: added proper checking of access and share on file stream constr…
HarrisonTCodes Nov 14, 2025
543acdb
test: added unit tests to cover simultaneous file stream opening with…
HarrisonTCodes Nov 14, 2025
795891b
fix: used explicit GUID call instead of target-typed new for clarity …
HarrisonTCodes Nov 14, 2025
42a7402
Merge branch 'main' into fix/file-stream-sharing
HarrisonTCodes Nov 14, 2025
c689d74
chore: explicit API acceptance test changes for dotnet version 10
HarrisonTCodes Nov 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/TestableIO.System.IO.Abstractions.TestingHelpers/MockFile.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -677,7 +677,7 @@ private FileSystemStream OpenInternal(
}
mockFileDataAccessor.AdjustTimes(mockFileData, timeAdjustments);

return new MockFileStream(mockFileDataAccessor, path, mode, access, options);
return new MockFileStream(mockFileDataAccessor, path, mode, access, FileShare.Read, options);
}

/// <inheritdoc />
Expand Down
21 changes: 21 additions & 0 deletions src/TestableIO.System.IO.Abstractions.TestingHelpers/MockFileStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using System.Threading;
using System.Runtime.Versioning;
using System.Security.AccessControl;
using System.Collections.Concurrent;

namespace System.IO.Abstractions.TestingHelpers;

Expand Down Expand Up @@ -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),
Expand All @@ -54,6 +58,11 @@ public MockFileStream(
this.path = path;
this.options = options;

if (_fileShareNoneStreams.ContainsKey(path))
Copy link
Contributor

@hangy hangy Oct 31, 2025

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 mockFileDataAccessor does like this:

System.IO.Abstractions/src/TestableIO.System.IO.Abstractions.TestingHelpers/MockFileStream.cs

Line 57 in 72ea54d

if (mockFileDataAccessor.FileExists(path))

System.IO.Abstractions/src/TestableIO.System.IO.Abstractions.TestingHelpers/MockFileSystem.cs

Line 476 in 72ea54d

path = FixPath(path).TrimSlashes();

System.IO.Abstractions/src/TestableIO.System.IO.Abstractions.TestingHelpers/MockFileSystem.cs

Lines 131 to 142 in 72ea54d

private string FixPath(string path, bool checkCaps = false)
{
if (path == null)
{
throw new ArgumentNullException(nameof(path), StringResources.Manager.GetString("VALUE_CANNOT_BE_NULL"));
}
var pathSeparatorFixed = path.Replace(Path.AltDirectorySeparatorChar, Path.DirectorySeparatorChar);
var fullPath = Path.GetFullPath(pathSeparatorFixed);
return checkCaps ? GetPathWithCorrectDirectoryCapitalization(fullPath) : fullPath;
}

Copy link
Contributor Author

@HarrisonTCodes HarrisonTCodes Oct 31, 2025 •
edited
Loading

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 NormalizePath method to the MockFileSystem class 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 FixPath method somewhere (or potentially just making FixPath public?), 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!

Copy link
Member

@vbreuss vbreuss Nov 9, 2025

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.

Copy link
Contributor Author

@HarrisonTCodes HarrisonTCodes Nov 9, 2025

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.

Agreed. I have been digging around the codebase, and it seems to me a good approach to this would be to move the FixPath method (and associated other methods) logic from the MockFileSystem class into the PathVerifier class, updating both MockFileSystem and MockFileStream accordingly?

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!

Copy link
Member

@vbreuss vbreuss Nov 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds reasonable...

Copy link
Contributor Author

@HarrisonTCodes HarrisonTCodes Nov 9, 2025

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!

Copy link
Member

@vbreuss vbreuss Nov 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now works only for FileShare.None, but not for read and write access:

Access Share Result
Read Read ok
Read ReadWrite ok
Read Write throw
Read None throw
Write Read throw
Write ReadWrite ok
Write Write ok
Write None throw
ReadWrite Read throw
ReadWrite ReadWrite ok
ReadWrite Write throw
ReadWrite None throw

I think it would be a great idea to also handle these cases correctly. What do you think?

Copy link
Contributor Author

@HarrisonTCodes HarrisonTCodes Nov 9, 2025

Choose a reason for hiding this comment

The 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 ConcurrentDictionary as effectively a concurrent hashmap, we actually use the value of entries to store the share they were opened with, and check against it where we currently just check for existence.

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!

Copy link
Member

@vbreuss vbreuss Nov 9, 2025

Choose a reason for hiding this comment

The 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 ConcurrentDictionary<IStorageLocation, ConcurrentDictionary<Guid, FileHandle>> with a custom FileHandle class (see here).

Copy link
Contributor Author

@HarrisonTCodes HarrisonTCodes Nov 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, multiple streams of the same file open at once...
Thanks for the reference, I'll checkout how its handled in Testably.Abstractions, maybe some of it could be directly ported over?

Copy link
Contributor Author

@HarrisonTCodes HarrisonTCodes Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I have implemented this functionality and the correct handling of the cases described above in this thread, currently using ConcurrentDictionary<string, ConcurrentDictionary<Guid, (FileAccess access, FileShare share)>> (behaviour covered in MockFileStreamTests.cs). Do review when you get the chance and let me know any pain points, things I may have missed, or desired changes! (maybe a type alias/class like Testably's FileHandle for the access/share tuple...)

{
throw CommonExceptions.ProcessCannotAccessFileInUse(path);
}

if (mockFileDataAccessor.FileExists(path))
{
if (mode.Equals(FileMode.CreateNew))
Expand Down Expand Up @@ -97,7 +106,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)
Expand Down Expand Up @@ -144,6 +161,10 @@ protected override void Dispose(bool disposing)
{
return;
}
if (share is FileShare.None)
{
_fileShareNoneStreams.TryRemove(path, out _);
}
InternalFlush();
base.Dispose(disposing);
OnClose();
Expand Down
10 changes: 5 additions & 5 deletions src/TestableIO.System.IO.Abstractions.TestingHelpers/MockFileStreamFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,25 +41,25 @@ public FileSystemStream New(string path, FileMode mode, FileAccess access)

/// <inheritdoc />
public FileSystemStream New(string path, FileMode mode, FileAccess access, FileShare share)
=> new MockFileStream(mockFileSystem, path, mode, access);
=> new MockFileStream(mockFileSystem, path, mode, access, share);

/// <inheritdoc />
public FileSystemStream New(string path, FileMode mode, FileAccess access, FileShare share, int bufferSize)
=> new MockFileStream(mockFileSystem, path, mode, access);
=> new MockFileStream(mockFileSystem, path, mode, access, share);

/// <inheritdoc />
public FileSystemStream New(string path, FileMode mode, FileAccess access, FileShare share, int bufferSize, bool useAsync)
=> new MockFileStream(mockFileSystem, path, mode, access);
=> new MockFileStream(mockFileSystem, path, mode, access, share);

/// <inheritdoc />
public FileSystemStream New(string path, FileMode mode, FileAccess access, FileShare share, int bufferSize,
FileOptions options)
=> new MockFileStream(mockFileSystem, path, mode, access, options);
=> new MockFileStream(mockFileSystem, path, mode, access, share, options);

#if FEATURE_FILESTREAM_OPTIONS
/// <inheritdoc />
public FileSystemStream New(string path, FileStreamOptions options)
=> new MockFileStream(mockFileSystem, path, options.Mode, options.Access, options.Options);
=> new MockFileStream(mockFileSystem, path, options.Mode, options.Access, options: options.Options);
#endif

/// <inheritdoc />
Expand Down
2 changes: 1 addition & 1 deletion ...stractions.Api.Tests/Expected/TestableIO.System.IO.Abstractions.TestingHelpers_net472.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ namespace System.IO.Abstractions.TestingHelpers
[System.Serializable]
public class MockFileStream : System.IO.Abstractions.FileSystemStream, System.IO.Abstractions.IFileSystemAclSupport
{
public MockFileStream(System.IO.Abstractions.TestingHelpers.IMockFileDataAccessor mockFileDataAccessor, string path, System.IO.FileMode mode, System.IO.FileAccess access = 3, System.IO.FileOptions options = 0) { }
public MockFileStream(System.IO.Abstractions.TestingHelpers.IMockFileDataAccessor mockFileDataAccessor, string path, System.IO.FileMode mode, System.IO.FileAccess access = 3, System.IO.FileShare share = 1, System.IO.FileOptions options = 0) { }
public override bool CanRead { get; }
public override bool CanWrite { get; }
public static System.IO.Abstractions.FileSystemStream Null { get; }
Expand Down
2 changes: 1 addition & 1 deletion ...stractions.Api.Tests/Expected/TestableIO.System.IO.Abstractions.TestingHelpers_net6.0.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ namespace System.IO.Abstractions.TestingHelpers
[System.Serializable]
public class MockFileStream : System.IO.Abstractions.FileSystemStream, System.IO.Abstractions.IFileSystemAclSupport
{
public MockFileStream(System.IO.Abstractions.TestingHelpers.IMockFileDataAccessor mockFileDataAccessor, string path, System.IO.FileMode mode, System.IO.FileAccess access = 3, System.IO.FileOptions options = 0) { }
public MockFileStream(System.IO.Abstractions.TestingHelpers.IMockFileDataAccessor mockFileDataAccessor, string path, System.IO.FileMode mode, System.IO.FileAccess access = 3, System.IO.FileShare share = 1, System.IO.FileOptions options = 0) { }
public override bool CanRead { get; }
public override bool CanWrite { get; }
public static System.IO.Abstractions.FileSystemStream Null { get; }
Expand Down
2 changes: 1 addition & 1 deletion ...stractions.Api.Tests/Expected/TestableIO.System.IO.Abstractions.TestingHelpers_net8.0.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ namespace System.IO.Abstractions.TestingHelpers
[System.Serializable]
public class MockFileStream : System.IO.Abstractions.FileSystemStream, System.IO.Abstractions.IFileSystemAclSupport
{
public MockFileStream(System.IO.Abstractions.TestingHelpers.IMockFileDataAccessor mockFileDataAccessor, string path, System.IO.FileMode mode, System.IO.FileAccess access = 3, System.IO.FileOptions options = 0) { }
public MockFileStream(System.IO.Abstractions.TestingHelpers.IMockFileDataAccessor mockFileDataAccessor, string path, System.IO.FileMode mode, System.IO.FileAccess access = 3, System.IO.FileShare share = 1, System.IO.FileOptions options = 0) { }
public override bool CanRead { get; }
public override bool CanWrite { get; }
public static System.IO.Abstractions.FileSystemStream Null { get; }
Expand Down
2 changes: 1 addition & 1 deletion ...stractions.Api.Tests/Expected/TestableIO.System.IO.Abstractions.TestingHelpers_net9.0.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ namespace System.IO.Abstractions.TestingHelpers
[System.Serializable]
public class MockFileStream : System.IO.Abstractions.FileSystemStream, System.IO.Abstractions.IFileSystemAclSupport
{
public MockFileStream(System.IO.Abstractions.TestingHelpers.IMockFileDataAccessor mockFileDataAccessor, string path, System.IO.FileMode mode, System.IO.FileAccess access = 3, System.IO.FileOptions options = 0) { }
public MockFileStream(System.IO.Abstractions.TestingHelpers.IMockFileDataAccessor mockFileDataAccessor, string path, System.IO.FileMode mode, System.IO.FileAccess access = 3, System.IO.FileShare share = 1, System.IO.FileOptions options = 0) { }
public override bool CanRead { get; }
public override bool CanWrite { get; }
public static System.IO.Abstractions.FileSystemStream Null { get; }
Expand Down
2 changes: 1 addition & 1 deletion ...ns.Api.Tests/Expected/TestableIO.System.IO.Abstractions.TestingHelpers_netstandard2.0.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ namespace System.IO.Abstractions.TestingHelpers
[System.Serializable]
public class MockFileStream : System.IO.Abstractions.FileSystemStream, System.IO.Abstractions.IFileSystemAclSupport
{
public MockFileStream(System.IO.Abstractions.TestingHelpers.IMockFileDataAccessor mockFileDataAccessor, string path, System.IO.FileMode mode, System.IO.FileAccess access = 3, System.IO.FileOptions options = 0) { }
public MockFileStream(System.IO.Abstractions.TestingHelpers.IMockFileDataAccessor mockFileDataAccessor, string path, System.IO.FileMode mode, System.IO.FileAccess access = 3, System.IO.FileShare share = 1, System.IO.FileOptions options = 0) { }
public override bool CanRead { get; }
public override bool CanWrite { get; }
public static System.IO.Abstractions.FileSystemStream Null { get; }
Expand Down
2 changes: 1 addition & 1 deletion ...ns.Api.Tests/Expected/TestableIO.System.IO.Abstractions.TestingHelpers_netstandard2.1.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ namespace System.IO.Abstractions.TestingHelpers
[System.Serializable]
public class MockFileStream : System.IO.Abstractions.FileSystemStream, System.IO.Abstractions.IFileSystemAclSupport
{
public MockFileStream(System.IO.Abstractions.TestingHelpers.IMockFileDataAccessor mockFileDataAccessor, string path, System.IO.FileMode mode, System.IO.FileAccess access = 3, System.IO.FileOptions options = 0) { }
public MockFileStream(System.IO.Abstractions.TestingHelpers.IMockFileDataAccessor mockFileDataAccessor, string path, System.IO.FileMode mode, System.IO.FileAccess access = 3, System.IO.FileShare share = 1, System.IO.FileOptions options = 0) { }
public override bool CanRead { get; }
public override bool CanWrite { get; }
public static System.IO.Abstractions.FileSystemStream Null { get; }
Expand Down
22 changes: 22 additions & 0 deletions tests/TestableIO.System.IO.Abstractions.TestingHelpers.Tests/MockFileStreamTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>();
}
}

[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();
}
}