Skip to content

fix: Fixes TestableIO/System.IO.Abstractions#1131 #1312

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mistial-dev
Copy link

The MockFileStream class does not handle shared file contents correctly when multiple streams are opened on the same file with FileShare.ReadWrite. Changes written through one stream were not visible to other streams, unlike real filesystem behavior.

This patch adds guards and versioning to handle the shared state in the same way a real filesystem would.

Made unit test deterministic for CI/CD
@vbreuss vbreuss requested a review from Copilot July 13, 2025 10:24
Copilot

This comment was marked as outdated.

@mistial-dev
Copy link
Author

I started down this path to try to make the fix thread-safe (for #784), but there's a number of complications in doing this a good way.

I'm looking at the race condition that was mentioned mentioned - where a file gets deleted between checking it exists and actually getting it. Should be simple? Just make it atomic. But then I fell down the rabbit hole...

Turns out MockFileStreamFactory just... ignores the FileShare parameter completely? Like, you pass in FileShare.None expecting exclusive access, but nope - the factory just throws it away and creates the stream without it.

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

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

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

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

This breaks a bunch of stuff, in terms of more complicated testing and proper emulation of filesystems.

Should I keep going with this? It's way bigger than just fixing the null reference issue, but without it, the mock file system is kinda broken for any serious concurrent file access testing.

@vbreuss
Copy link
Member

vbreuss commented Jul 13, 2025

Did you per chance check, if you have the same problems with the MockFileSystem from Testably.Abstractions, @mistial-dev? There I implemented checks for FileMode and FileAccess...

… and handle unflushed writes. Split a function to reduce complexity. Fixed code sniffs (especially insecure file path generation). Updated API baseline due to Read() and Length override. This should not be a breaking change.
@mistial-dev
Copy link
Author

Did you per chance check, if you have the same problems with the MockFileSystem from Testably.Abstractions, @mistial-dev? There I implemented checks for FileMode and FileAccess...

I haven't gotten that far yet. I was working on the sniffs and API stuff for this one.

@mistial-dev mistial-dev requested a review from Copilot July 13, 2025 11:42
Copilot

This comment was marked as outdated.

@mistial-dev mistial-dev requested a review from Copilot July 15, 2025 08:09
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes issue #1131 by implementing version-based synchronization in MockFileStream so that multiple streams with FileShare.ReadWrite see each other’s changes, and adds tests to verify this behavior.

  • Introduces lastKnownContentVersion and hasUnflushedWrites in MockFileStream for change tracking
  • Implements RefreshFromSharedContentIfNeeded, merging logic, and InternalFlush to sync content
  • Adds numerous tests in MockFileStreamTests covering shared reads, writes, truncation, and concurrent scenarios
  • Updates API test snapshots to include the new Length override and Read(Span<byte>) override

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/TestableIO.System.IO.Abstractions.TestingHelpers.Tests/MockFileStreamTests.cs Add tests for shared file content visibility; validate reads/writes across streams
tests/TestableIO.System.IO.Abstractions.Api.Tests/Expected/TestableIO.System.IO.Abstractions.TestingHelpers_netstandard2.1.txt Update expected API to include Length and Read(Span<byte>) overrides
tests/TestableIO.System.IO.Abstractions.Api.Tests/Expected/TestableIO.System.IO.Abstractions.TestingHelpers_netstandard2.0.txt Update expected API to include Length override
tests/TestableIO.System.IO.Abstractions.Api.Tests/Expected/TestableIO.System.IO.Abstractions.TestingHelpers_net9.0.txt Update expected API to include Length and Read(Span<byte>) overrides
tests/TestableIO.System.IO.Abstractions.Api.Tests/Expected/TestableIO.System.IO.Abstractions.TestingHelpers_net8.0.txt Update expected API to include Length and Read(Span<byte>) overrides
tests/TestableIO.System.IO.Abstractions.Api.Tests/Expected/TestableIO.System.IO.Abstractions.TestingHelpers_net6.0.txt Update expected API to include Length and Read(Span<byte>) overrides
tests/TestableIO.System.IO.Abstractions.Api.Tests/Expected/TestableIO.System.IO.Abstractions.TestingHelpers_net472.txt Update expected API to include Length override
src/TestableIO.System.IO.Abstractions.TestingHelpers/MockFileStream.cs Add version checks, merge/unflushed write preservation, and InternalFlush logic
src/TestableIO.System.IO.Abstractions.TestingHelpers/MockFileData.cs Introduce contentVersion tracking and update Contents setter to increment it
Comments suppressed due to low confidence (3)

tests/TestableIO.System.IO.Abstractions.TestingHelpers.Tests/MockFileStreamTests.cs:3

  • These using directives are missing the System. prefix. Update to using System.Collections.Generic;
using Collections.Generic;

tests/TestableIO.System.IO.Abstractions.TestingHelpers.Tests/MockFileStreamTests.cs:4

  • This using directive should be using System.Linq; to access LINQ extension methods like Take and SequenceEqual.
using Linq;

tests/TestableIO.System.IO.Abstractions.TestingHelpers.Tests/MockFileStreamTests.cs:5

  • This using directive should be using System.Threading.Tasks; to resolve Task and async/await support.
using Threading.Tasks;

@vbreuss
Copy link
Member

vbreuss commented Jul 19, 2025

@mistial-dev: What's the status of this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants