Skip to content
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

Jump to bottom

fix: ensure correct behavior when moving or replacing a file with an open handle #555

Merged
merged 3 commits into from
Apr 9, 2024
Merged

fix: ensure correct behavior when moving or replacing a file with an open handle #555

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d8ec669
Create initial Move tests to determine the actual behavior
vbreuss Apr 8, 2024
71dba3e
Adapt tests for Copy, Move and Replace for File and FileInfo
vbreuss Apr 8, 2024
c4eaec9
Ignore FileShare for Linux and MacOS when calling Move or Replace met…
vbreuss Apr 9, 2024
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
1 change: 1 addition & 0 deletions Source/Testably.Abstractions.Testing/Storage/IStorageContainer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ internal interface IStorageContainer : IFileSystemEntity, ITimeSystemEntity
/// <returns>An <see cref="IStorageAccessHandle" /> that is used to release the access lock on dispose.</returns>
IStorageAccessHandle RequestAccess(FileAccess access, FileShare share,
bool deleteAccess = false,
bool ignoreFileShare = false,
bool ignoreMetadataErrors = true,
int? hResult = null);

Expand Down
16 changes: 10 additions & 6 deletions Source/Testably.Abstractions.Testing/Storage/InMemoryContainer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,10 @@ public void Encrypt()
/// <inheritdoc cref="IStorageContainer.GetBytes()" />
public byte[] GetBytes() => _bytes;

/// <inheritdoc cref="IStorageContainer.RequestAccess(FileAccess, FileShare, bool, bool, int?)" />
/// <inheritdoc cref="IStorageContainer.RequestAccess(FileAccess, FileShare, bool, bool, bool, int?)" />
public IStorageAccessHandle RequestAccess(FileAccess access, FileShare share,
bool deleteAccess = false,
bool ignoreFileShare = false,
bool ignoreMetadataErrors = true,
int? hResult = null)
{
Expand All @@ -163,7 +164,7 @@ public IStorageAccessHandle RequestAccess(FileAccess access, FileShare share,
throw ExceptionFactory.AclAccessToPathDenied(_location.FullPath);
}

if (CanGetAccess(access, share, deleteAccess))
if (CanGetAccess(access, share, deleteAccess, ignoreFileShare))
{
Guid guid = Guid.NewGuid();
FileHandle fileHandle =
Expand Down Expand Up @@ -289,11 +290,11 @@ internal FileAttributes AdjustAttributes(FileAttributes attributes)
return attributes;
}

private bool CanGetAccess(FileAccess access, FileShare share, bool deleteAccess)
private bool CanGetAccess(FileAccess access, FileShare share, bool deleteAccess, bool ignoreFileShare)
{
foreach (KeyValuePair<Guid, FileHandle> fileHandle in _fileHandles)
{
if (!fileHandle.Value.GrantAccess(access, share, deleteAccess))
if (!fileHandle.Value.GrantAccess(access, share, deleteAccess, ignoreFileShare))
{
return false;
}
Expand Down Expand Up @@ -385,17 +386,20 @@ public void Dispose()

#endregion

public bool GrantAccess(FileAccess access, FileShare share, bool deleteAccess)
public bool GrantAccess(FileAccess access, FileShare share, bool deleteAccess, bool ignoreFileShare)
{
FileShare usedShare = share;
FileShare currentShare = Share;
_fileSystem.Execute.NotOnWindows(()
=> usedShare = FileShare.ReadWrite);
_fileSystem.Execute.NotOnWindowsIf(ignoreFileShare, ()
=> currentShare = FileShare.ReadWrite);
if (deleteAccess)
{
return !_fileSystem.Execute.IsWindows || Share == FileShare.Delete;
}

return CheckAccessWithShare(access, Share) &&
return CheckAccessWithShare(access, currentShare) &&
CheckAccessWithShare(Access, usedShare);
}

Expand Down
7 changes: 5 additions & 2 deletions Source/Testably.Abstractions.Testing/Storage/InMemoryStorage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,12 +409,14 @@ public IStorageContainer GetOrCreateContainer(
using (_ = sourceContainer.RequestAccess(
FileAccess.ReadWrite,
FileShare.None,
ignoreMetadataErrors: ignoreMetadataErrors))
ignoreMetadataErrors: ignoreMetadataErrors,
ignoreFileShare: true))
{
using (_ = destinationContainer.RequestAccess(
FileAccess.ReadWrite,
FileShare.None,
ignoreMetadataErrors: ignoreMetadataErrors))
ignoreMetadataErrors: ignoreMetadataErrors,
ignoreFileShare: true))
{
if (_containers.TryRemove(destination,
out IStorageContainer? existingDestinationContainer))
Expand Down Expand Up @@ -664,6 +666,7 @@ private void CreateParents(MockFileSystem fileSystem, IStorageLocation location)
}

using (container.RequestAccess(FileAccess.Write, FileShare.None,
ignoreFileShare: true,
hResult: sourceType == FileSystemTypes.Directory ? -2147024891 : -2147024864))
{
if (children.Any() && recursive)
Expand Down
3 changes: 2 additions & 1 deletion Source/Testably.Abstractions.Testing/Storage/NullContainer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,10 @@ public void Encrypt()
public byte[] GetBytes()
=> Array.Empty<byte>();

/// <inheritdoc cref="IStorageContainer.RequestAccess(FileAccess, FileShare, bool, bool, int?)" />
/// <inheritdoc cref="IStorageContainer.RequestAccess(FileAccess, FileShare, bool, bool, bool, int?)" />
public IStorageAccessHandle RequestAccess(FileAccess access, FileShare share,
bool deleteAccess = false,
bool ignoreFileShare = false,
bool ignoreMetadataErrors = true,
int? hResult = null)
=> new NullStorageAccessHandle(access, share, deleteAccess);
Expand Down
7 changes: 4 additions & 3 deletions Tests/Testably.Abstractions.Testing.Tests/TestHelpers/LockableContainer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ namespace Testably.Abstractions.Testing.Tests.TestHelpers;
/// A <see cref="IStorageContainer" /> for testing purposes.
/// <para />
/// Set <see cref="IsLocked" /> to <see langword="true" /> to simulate a locked file
/// (<see cref="RequestAccess(FileAccess, FileShare, bool, bool, int?)" /> throws an <see cref="IOException" />).
/// (<see cref="RequestAccess(FileAccess, FileShare, bool, bool, bool, int?)" /> throws an <see cref="IOException" />).
/// </summary>
internal sealed class LockableContainer(
MockFileSystem fileSystem,
Expand All @@ -24,7 +24,7 @@ internal sealed class LockableContainer(

/// <summary>
/// Simulate a locked file, if set to <see langword="true" />.<br />
/// In this case <see cref="RequestAccess(FileAccess, FileShare, bool, bool, int?)" /> throws
/// In this case <see cref="RequestAccess(FileAccess, FileShare, bool, bool, bool, int?)" /> throws
/// an <see cref="IOException" />, otherwise it will succeed.
/// </summary>
public bool IsLocked { get; set; }
Expand Down Expand Up @@ -89,9 +89,10 @@ public void Encrypt()
public byte[] GetBytes()
=> _bytes;

/// <inheritdoc cref="IStorageContainer.RequestAccess(FileAccess, FileShare, bool, bool, int?)" />
/// <inheritdoc cref="IStorageContainer.RequestAccess(FileAccess, FileShare, bool, bool, bool, int?)" />
public IStorageAccessHandle RequestAccess(FileAccess access, FileShare share,
bool deleteAccess = false,
bool ignoreFileShare = false,
bool ignoreMetadataErrors = true,
int? hResult = null)
{
Expand Down
36 changes: 28 additions & 8 deletions Tests/Testably.Abstractions.Tests/FileSystem/File/CopyTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,22 +266,17 @@ public void Copy_ShouldCopyFileWithContent(
[SkippableTheory]
[InlineAutoData(FileAccess.Read, FileShare.Read)]
[InlineAutoData(FileAccess.Read, FileShare.ReadWrite)]
[InlineAutoData(FileAccess.Read, FileShare.Write)]
[InlineAutoData(FileAccess.ReadWrite, FileShare.Read)]
[InlineAutoData(FileAccess.ReadWrite, FileShare.ReadWrite)]
[InlineAutoData(FileAccess.ReadWrite, FileShare.Write)]
[InlineAutoData(FileAccess.Write, FileShare.Read)]
[InlineAutoData(FileAccess.Write, FileShare.ReadWrite)]
[InlineAutoData(FileAccess.Write, FileShare.Write)]
public void Copy_SourceAccessedWithReadShare_ShouldNotThrow(
FileAccess fileAccess,
FileShare fileShare,
string sourcePath,
string destinationPath,
string sourceContents)
{
Skip.If(Test.RunsOnWindows && fileShare == FileShare.Write);

FileSystem.Initialize().WithFile(sourcePath)
.Which(f => f.HasStringContent(sourceContents));
using (FileSystem.FileStream
Expand All @@ -294,6 +289,30 @@ public void Copy_SourceAccessedWithReadShare_ShouldNotThrow(
FileSystem.File.ReadAllText(destinationPath).Should().Be(sourceContents);
}

[SkippableTheory]
[InlineAutoData(FileAccess.Read)]
[InlineAutoData(FileAccess.ReadWrite)]
[InlineAutoData(FileAccess.Write)]
public void Copy_SourceAccessedWithWriteShare_ShouldNotThrowOnLinuxOrMac(
FileAccess fileAccess,
string sourcePath,
string destinationPath,
string sourceContents)
{
Skip.If(Test.RunsOnWindows, "see https://github.com/dotnet/runtime/issues/52700");

FileSystem.Initialize().WithFile(sourcePath)
.Which(f => f.HasStringContent(sourceContents));
using (FileSystem.FileStream
.New(sourcePath, FileMode.Open, fileAccess, FileShare.Write))
{
FileSystem.File.Copy(sourcePath, destinationPath);
}

FileSystem.File.Exists(destinationPath).Should().BeTrue();
FileSystem.File.ReadAllText(destinationPath).Should().Be(sourceContents);
}

[SkippableTheory]
[AutoData]
public void Copy_SourceIsDirectory_ShouldThrowUnauthorizedAccessException_AndNotCopyFile(
Expand Down Expand Up @@ -324,11 +343,12 @@ public void Copy_SourceLocked_ShouldThrowIOException(
string sourceName,
string destinationName)
{
Skip.If(!Test.RunsOnWindows && fileShare == FileShare.Write);
Skip.If(!Test.RunsOnWindows && fileShare == FileShare.Write,
"see https://github.com/dotnet/runtime/issues/52700");

FileSystem.File.WriteAllText(sourceName, null);
using FileSystemStream stream = FileSystem.File.Open(sourceName, FileMode.Open,
FileAccess.Read, fileShare);
using FileSystemStream stream = FileSystem.File.Open(
sourceName, FileMode.Open, FileAccess.Read, fileShare);

Exception? exception = Record.Exception(() =>
{
Expand Down
24 changes: 19 additions & 5 deletions Tests/Testably.Abstractions.Tests/FileSystem/File/MoveTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,14 +181,27 @@ public void Move_SourceDirectoryMissing_ShouldThrowFileNotFoundException(
}

[SkippableTheory]
[AutoData]
[InlineAutoData(FileAccess.Read, FileShare.None)]
[InlineAutoData(FileAccess.Read, FileShare.Read)]
[InlineAutoData(FileAccess.Read, FileShare.ReadWrite)]
[InlineAutoData(FileAccess.Read, FileShare.Write)]
[InlineAutoData(FileAccess.ReadWrite, FileShare.None)]
[InlineAutoData(FileAccess.ReadWrite, FileShare.Read)]
[InlineAutoData(FileAccess.ReadWrite, FileShare.ReadWrite)]
[InlineAutoData(FileAccess.ReadWrite, FileShare.Write)]
[InlineAutoData(FileAccess.Write, FileShare.None)]
[InlineAutoData(FileAccess.Write, FileShare.Read)]
[InlineAutoData(FileAccess.Write, FileShare.ReadWrite)]
[InlineAutoData(FileAccess.Write, FileShare.Write)]
public void Move_SourceLocked_ShouldThrowIOException_OnWindows(
FileAccess fileAccess,
FileShare fileShare,
string sourceName,
string destinationName)
{
FileSystem.File.WriteAllText(sourceName, null);
using FileSystemStream stream = FileSystem.File.Open(sourceName, FileMode.Open,
FileAccess.Read, FileShare.Read);
using FileSystemStream stream = FileSystem.File.Open(
sourceName, FileMode.Open, fileAccess, fileShare);

Exception? exception = Record.Exception(() =>
{
Expand All @@ -197,12 +210,13 @@ public void Move_SourceLocked_ShouldThrowIOException_OnWindows(

if (Test.RunsOnWindows)
{
exception.Should().BeException<IOException>(
hResult: -2147024864);
exception.Should().BeException<IOException>(hResult: -2147024864);
FileSystem.Should().HaveFile(sourceName);
FileSystem.Should().NotHaveFile(destinationName);
}
else
{
// https://github.com/dotnet/runtime/issues/52700
FileSystem.Should().NotHaveFile(sourceName);
FileSystem.Should().HaveFile(destinationName);
}
Expand Down
20 changes: 17 additions & 3 deletions Tests/Testably.Abstractions.Tests/FileSystem/File/ReplaceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,21 @@ public void Replace_SourceIsDirectory_ShouldThrowUnauthorizedAccessException(
}

[SkippableTheory]
[AutoData]
[InlineAutoData(FileAccess.Read, FileShare.None)]
[InlineAutoData(FileAccess.Read, FileShare.Read)]
[InlineAutoData(FileAccess.Read, FileShare.ReadWrite)]
[InlineAutoData(FileAccess.Read, FileShare.Write)]
[InlineAutoData(FileAccess.ReadWrite, FileShare.None)]
[InlineAutoData(FileAccess.ReadWrite, FileShare.Read)]
[InlineAutoData(FileAccess.ReadWrite, FileShare.ReadWrite)]
[InlineAutoData(FileAccess.ReadWrite, FileShare.Write)]
[InlineAutoData(FileAccess.Write, FileShare.None)]
[InlineAutoData(FileAccess.Write, FileShare.Read)]
[InlineAutoData(FileAccess.Write, FileShare.ReadWrite)]
[InlineAutoData(FileAccess.Write, FileShare.Write)]
public void Replace_SourceLocked_ShouldThrowIOException_OnWindows(
FileAccess fileAccess,
FileShare fileShare,
string sourceName,
string destinationName,
string backupName,
Expand All @@ -186,8 +199,8 @@ public void Replace_SourceLocked_ShouldThrowIOException_OnWindows(
FileSystem.File.WriteAllText(destinationName, destinationContents);

Exception? exception;
using (FileSystemStream _ = FileSystem.File.Open(sourceName,
FileMode.Open, FileAccess.Read, FileShare.Read))
using (FileSystemStream _ = FileSystem.File.Open(
sourceName, FileMode.Open, fileAccess, fileShare))
{
exception = Record.Exception(() =>
{
Expand All @@ -208,6 +221,7 @@ public void Replace_SourceLocked_ShouldThrowIOException_OnWindows(
}
else
{
// https://github.com/dotnet/runtime/issues/52700
FileSystem.Should().NotHaveFile(sourceName);
FileSystem.Should().HaveFile(destinationName)
.Which.HasContent(sourceContents);
Expand Down
61 changes: 59 additions & 2 deletions Tests/Testably.Abstractions.Tests/FileSystem/FileInfo/CopyToTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,58 @@ public void CopyTo_ShouldKeepMetadata(
.Should().Be(sourceLastWriteTime);
}

[SkippableTheory]
[InlineAutoData(FileAccess.Read, FileShare.Read)]
[InlineAutoData(FileAccess.Read, FileShare.ReadWrite)]
[InlineAutoData(FileAccess.ReadWrite, FileShare.Read)]
[InlineAutoData(FileAccess.ReadWrite, FileShare.ReadWrite)]
[InlineAutoData(FileAccess.Write, FileShare.Read)]
[InlineAutoData(FileAccess.Write, FileShare.ReadWrite)]
public void CopyTo_SourceAccessedWithReadShare_ShouldNotThrow(
FileAccess fileAccess,
FileShare fileShare,
string sourcePath,
string destinationPath,
string sourceContents)
{
FileSystem.Initialize().WithFile(sourcePath)
.Which(f => f.HasStringContent(sourceContents));
IFileInfo sut = FileSystem.FileInfo.New(sourcePath);
using (FileSystem.FileStream
.New(sourcePath, FileMode.Open, fileAccess, fileShare))
{
sut.CopyTo(destinationPath);
}

FileSystem.File.Exists(destinationPath).Should().BeTrue();
FileSystem.File.ReadAllText(destinationPath).Should().Be(sourceContents);
}

[SkippableTheory]
[InlineAutoData(FileAccess.Read)]
[InlineAutoData(FileAccess.ReadWrite)]
[InlineAutoData(FileAccess.Write)]
public void CopyTo_SourceAccessedWithWriteShare_ShouldNotThrowOnLinuxOrMac(
FileAccess fileAccess,
string sourcePath,
string destinationPath,
string sourceContents)
{
Skip.If(Test.RunsOnWindows, "see https://github.com/dotnet/runtime/issues/52700");

FileSystem.Initialize().WithFile(sourcePath)
.Which(f => f.HasStringContent(sourceContents));
IFileInfo sut = FileSystem.FileInfo.New(sourcePath);
using (FileSystem.FileStream
.New(sourcePath, FileMode.Open, fileAccess, FileShare.Write))
{
sut.CopyTo(destinationPath);
}

FileSystem.File.Exists(destinationPath).Should().BeTrue();
FileSystem.File.ReadAllText(destinationPath).Should().Be(sourceContents);
}

[SkippableTheory]
[AutoData]
public void CopyTo_SourceIsDirectory_ShouldThrowUnauthorizedAccessException_AndNotCopyFile(
Expand All @@ -195,14 +247,19 @@ public void CopyTo_SourceIsDirectory_ShouldThrowUnauthorizedAccessException_AndN
}

[SkippableTheory]
[AutoData]
[InlineAutoData(FileShare.None)]
[InlineAutoData(FileShare.Write)]
public void CopyTo_SourceLocked_ShouldThrowIOException(
FileShare fileShare,
string sourceName,
string destinationName)
{
Skip.If(!Test.RunsOnWindows && fileShare == FileShare.Write,
"see https://github.com/dotnet/runtime/issues/52700");

FileSystem.File.WriteAllText(sourceName, null);
using FileSystemStream stream = FileSystem.File.Open(sourceName, FileMode.Open,
FileAccess.Read, FileShare.None);
FileAccess.Read, fileShare);
IFileInfo sut = FileSystem.FileInfo.New(sourceName);

Exception? exception = Record.Exception(() =>
Expand Down
Loading
Loading