Skip to content

Commit

Permalink
FileStream rewrite part II (#48813)
Browse files Browse the repository at this point in the history
* introduce WindowsFileStreamStrategy

* introduce SyncWindowsFileStreamStrategy

* introduce AsyncWindowsFileStreamStrategy

* only DerivedFileStreamStrategy needs to have a reference to FileStream

remove finalizer from FileStream, keep it only in DerivedFileStreamStrategy and LegacyFileStreamStrategy

* use the new strategies when buffering is not enabled

* introduce BufferedFileStreamStrategy

* use the Legacy strategy by default for now, add tests for the other implementation

* Apply suggestions from code review

Co-authored-by: David Cantú <[email protected]>
Co-authored-by: Stephen Toub <[email protected]>
  • Loading branch information
3 people authored Mar 17, 2021
1 parent 1292216 commit 6ef4b2e
Show file tree
Hide file tree
Showing 43 changed files with 3,086 additions and 844 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -95,5 +95,7 @@ unsafe void IDeferredDisposable.OnFinalRelease(bool disposed)
}
}
}

internal bool IsUserObject(byte[]? buffer) => _overlapped.IsUserObject(buffer);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@ internal sealed unsafe class OverlappedData
return AllocateNativeOverlapped();
}

internal bool IsUserObject(byte[]? buffer) => ReferenceEquals(_userObject, buffer);

[MethodImpl(MethodImplOptions.InternalCall)]
private extern NativeOverlapped* AllocateNativeOverlapped();

Expand Down Expand Up @@ -258,6 +260,8 @@ public static unsafe void Free(NativeOverlapped* nativeOverlappedPtr)
OverlappedData.GetOverlappedFromNative(nativeOverlappedPtr)._overlapped._overlappedData = null;
OverlappedData.FreeNativeOverlapped(nativeOverlappedPtr);
}

internal bool IsUserObject(byte[]? buffer) => _overlappedData!.IsUserObject(buffer);
}

#endregion class Overlapped
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,14 +187,14 @@ public static string GetDistroVersionString()
}
}

private static readonly Lazy<bool> m_isInvariant = new Lazy<bool>(GetIsInvariantGlobalization);
private static readonly Lazy<bool> m_isInvariant = new Lazy<bool>(() => GetStaticNonPublicBooleanPropertyValue("System.Globalization.GlobalizationMode", "Invariant"));

private static bool GetIsInvariantGlobalization()
private static bool GetStaticNonPublicBooleanPropertyValue(string typeName, string propertyName)
{
Type globalizationMode = Type.GetType("System.Globalization.GlobalizationMode");
Type globalizationMode = Type.GetType(typeName);
if (globalizationMode != null)
{
MethodInfo methodInfo = globalizationMode.GetProperty("Invariant", BindingFlags.NonPublic | BindingFlags.Static)?.GetMethod;
MethodInfo methodInfo = globalizationMode.GetProperty(propertyName, BindingFlags.NonPublic | BindingFlags.Static)?.GetMethod;
if (methodInfo != null)
{
return (bool)methodInfo.Invoke(null, null);
Expand Down Expand Up @@ -235,6 +235,10 @@ private static Version GetICUVersion()
version & 0xFF);
}

private static readonly Lazy<bool> _legacyFileStream = new Lazy<bool>(() => GetStaticNonPublicBooleanPropertyValue("System.IO.FileStreamHelpers", "UseLegacyStrategy"));

public static bool IsLegacyFileStreamEnabled => _legacyFileStream.Value;

private static bool GetIsInContainer()
{
if (IsWindows)
Expand Down
7 changes: 7 additions & 0 deletions src/libraries/System.IO.FileSystem/System.IO.FileSystem.sln
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "ref", "ref", "{32A31E04-255
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "src", "src", "{D9FB1730-B750-4C0D-8D24-8C992DEB6034}"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "System.IO.FileSystem.Legacy.Tests", "tests\LegacyTests\System.IO.FileSystem.Legacy.Tests.csproj", "{48E07F12-8597-40DE-8A37-CCBEB9D54012}"
EndProject
Global
GlobalSection(NestedProjects) = preSolution
{D350D6E7-52F1-40A4-B646-C178F6BBB689} = {1A727AF9-4F39-4109-BB8F-813286031DC9}
Expand All @@ -37,6 +39,7 @@ Global
{06DD5AA8-FDA6-495B-A8D1-8CE83C78DE6C} = {32A31E04-2554-4223-BED8-45757408B4F6}
{877E39A8-51CB-463A-AF4C-6FAE4F438075} = {D9FB1730-B750-4C0D-8D24-8C992DEB6034}
{D7DF8034-3AE5-4DEF-BCC4-6353239391BF} = {D9FB1730-B750-4C0D-8D24-8C992DEB6034}
{48E07F12-8597-40DE-8A37-CCBEB9D54012} = {1A727AF9-4F39-4109-BB8F-813286031DC9}
EndGlobalSection
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Expand Down Expand Up @@ -83,6 +86,10 @@ Global
{06DD5AA8-FDA6-495B-A8D1-8CE83C78DE6C}.Debug|Any CPU.Build.0 = Debug|Any CPU
{06DD5AA8-FDA6-495B-A8D1-8CE83C78DE6C}.Release|Any CPU.ActiveCfg = Release|Any CPU
{06DD5AA8-FDA6-495B-A8D1-8CE83C78DE6C}.Release|Any CPU.Build.0 = Release|Any CPU
{48E07F12-8597-40DE-8A37-CCBEB9D54012}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{48E07F12-8597-40DE-8A37-CCBEB9D54012}.Debug|Any CPU.Build.0 = Debug|Any CPU
{48E07F12-8597-40DE-8A37-CCBEB9D54012}.Release|Any CPU.ActiveCfg = Release|Any CPU
{48E07F12-8597-40DE-8A37-CCBEB9D54012}.Release|Any CPU.Build.0 = Release|Any CPU
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,14 @@ public void DisposeClosesHandle()
}
}

[Fact]
public void DisposingBufferedFileStreamThatWasClosedViaSafeFileHandleCloseDoesNotThrow()
{
FileStream fs = new FileStream(GetTestFilePath(), FileMode.Create, FileAccess.ReadWrite, FileShare.Read, bufferSize: 100);
fs.SafeFileHandle.Dispose();
fs.Dispose(); // must not throw
}

[Fact]
public void AccessFlushesFileClosesHandle()
{
Expand Down Expand Up @@ -96,15 +104,13 @@ private async Task ThrowWhenHandlePositionIsChanged(bool useAsync)
// Put data in FS write buffer and update position from FSR
fs.WriteByte(0);
fsr.Position = 0;
Assert.Throws<IOException>(() => fs.Position);

fs.WriteByte(0);
fsr.Position++;
Assert.Throws<IOException>(() => fs.Read(new byte[1], 0, 1));

fs.WriteByte(0);
fsr.Position++;
if (useAsync && OperatingSystem.IsWindows()) // Async I/O behaviors differ due to kernel-based implementation on Windows
if (useAsync
// Async I/O behaviors differ due to kernel-based implementation on Windows
&& OperatingSystem.IsWindows()
// ReadAsync which in this case (single byte written to buffer) calls FlushAsync is now 100% async
// so it does not complete synchronously anymore
&& PlatformDetection.IsLegacyFileStreamEnabled)
{
Assert.Throws<IOException>(() => FSAssert.CompletesSynchronously(fs.ReadAsync(new byte[1], 0, 1)));
}
Expand All @@ -113,6 +119,14 @@ private async Task ThrowWhenHandlePositionIsChanged(bool useAsync)
await Assert.ThrowsAsync<IOException>(() => fs.ReadAsync(new byte[1], 0, 1));
}

fs.WriteByte(0);
fsr.Position++;
Assert.Throws<IOException>(() => fs.Read(new byte[1], 0, 1));

fs.WriteByte(0);
fsr.Position++;
await Assert.ThrowsAsync<IOException>(() => fs.ReadAsync(new byte[1], 0, 1));

fs.WriteByte(0);
fsr.Position++;
Assert.Throws<IOException>(() => fs.ReadByte());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,15 @@ public async Task ManyConcurrentWriteAsyncs_OuterLoop(
Assert.Null(writes[i].Exception);
if (useAsync)
{
Assert.Equal((i + 1) * writeSize, fs.Position);
// To ensure that the buffer of a FileStream opened for async IO is flushed
// by FlushAsync in asynchronous way, we aquire a lock for every buffered WriteAsync.
// The side effect of this is that the Position of FileStream is not updated until
// the lock is released by a previous operation.
// So now all WriteAsync calls should be awaited before starting another async file operation.
if (PlatformDetection.IsLegacyFileStreamEnabled)
{
Assert.Equal((i + 1) * writeSize, fs.Position);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Reflection;
using Xunit;

namespace System.IO.Tests
{
public class LegacySwitchTests
{
[Fact]
public static void LegacySwitchIsHonored()
{
string filePath = Path.Combine(Path.GetTempPath(), Path.GetTempFileName());

using (FileStream fileStream = File.Create(filePath))
{
object strategy = fileStream
.GetType()
.GetField("_strategy", BindingFlags.NonPublic | BindingFlags.Instance)
.GetValue(fileStream);

Assert.DoesNotContain(strategy.GetType().FullName, "Legacy");
}

File.Delete(filePath);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
<!-- currently we have non Legacy strategy only for Windows, so we run the tests only on Windows -->
<TargetFrameworks>$(NetCoreAppCurrent)-windows</TargetFrameworks>

<RunTestsJSArguments>--working-dir=/test-dir</RunTestsJSArguments>
</PropertyGroup>
<ItemGroup>
<Compile Include="..\**\*.cs" />
</ItemGroup>
<ItemGroup Condition="'$(TargetsUnix)' == 'true'">
<Compile Remove="..\**\*.Windows.cs" />
<Compile Remove="..\**\*.Browser.cs" />
</ItemGroup>
<ItemGroup Condition="'$(TargetsWindows)' == 'true'">
<Compile Remove="..\**\*.Unix.cs" />
<Compile Remove="..\**\*.Browser.cs" />
</ItemGroup>
<ItemGroup Condition="'$(TargetsBrowser)' == 'true'">
<Compile Remove="..\**\*.Unix.cs" />
<Compile Remove="..\**\*.Windows.cs" />
</ItemGroup>
<ItemGroup>
<!-- Helpers -->
<Compile Include="$(CommonPath)Interop\Windows\Kernel32\Interop.GenericOperations.cs" Link="Interop\Windows\Interop.GenericOperations.cs" />
<Compile Include="$(CommonTestPath)System\Buffers\NativeMemoryManager.cs" Link="Common\System\Buffers\NativeMemoryManager.cs" />
<Compile Include="$(CommonTestPath)System\IO\TempFile.cs" Link="Common\System\IO\TempFile.cs" />
<Compile Include="$(CommonTestPath)System\IO\PathFeatures.cs" Link="Common\System\IO\PathFeatures.cs" />
<Content Include="..\DirectoryInfo\test-dir\dummy.txt" Link="test-dir\dummy.txt" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="$(CommonTestPath)StreamConformanceTests\StreamConformanceTests.csproj" />
</ItemGroup>
</Project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"configProperties": {
"System.IO.UseLegacyFileStream": true
}
}
7 changes: 7 additions & 0 deletions src/libraries/System.IO/System.IO.sln
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "ref", "ref", "{9FDAA57A-696
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "src", "src", "{D9FD8082-D04C-4DA8-9F4C-261D1C65A6D3}"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "System.IO.Legacy.Tests", "tests\LegacyTests\System.IO.Legacy.Tests.csproj", "{0217540D-FA86-41B3-9754-7BB5096ABA3E}"
EndProject
Global
GlobalSection(NestedProjects) = preSolution
{D11D3624-1322-45D1-A604-7E68CDB85BE8} = {5AD2C433-C661-4AD1-BD9F-D164ADC43512}
Expand All @@ -34,6 +36,7 @@ Global
{D0D1CDAC-16F8-4382-A219-74A513CC1790} = {9FDAA57A-696B-4CB1-99AE-BCDF91848B75}
{0769544B-1A5D-4D74-94FD-899DF6C39D62} = {D9FD8082-D04C-4DA8-9F4C-261D1C65A6D3}
{AA5E80B2-A0AA-46F1-B319-5B528BAC382B} = {D9FD8082-D04C-4DA8-9F4C-261D1C65A6D3}
{0217540D-FA86-41B3-9754-7BB5096ABA3E} = {5AD2C433-C661-4AD1-BD9F-D164ADC43512}
EndGlobalSection
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Expand Down Expand Up @@ -76,6 +79,10 @@ Global
{D0D1CDAC-16F8-4382-A219-74A513CC1790}.Debug|Any CPU.Build.0 = Debug|Any CPU
{D0D1CDAC-16F8-4382-A219-74A513CC1790}.Release|Any CPU.ActiveCfg = Release|Any CPU
{D0D1CDAC-16F8-4382-A219-74A513CC1790}.Release|Any CPU.Build.0 = Release|Any CPU
{0217540D-FA86-41B3-9754-7BB5096ABA3E}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{0217540D-FA86-41B3-9754-7BB5096ABA3E}.Debug|Any CPU.Build.0 = Debug|Any CPU
{0217540D-FA86-41B3-9754-7BB5096ABA3E}.Release|Any CPU.ActiveCfg = Release|Any CPU
{0217540D-FA86-41B3-9754-7BB5096ABA3E}.Release|Any CPU.Build.0 = Release|Any CPU
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<RootNamespace>System.IO</RootNamespace>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<TestRuntime>true</TestRuntime>
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
<!-- currently we have non Legacy strategy only for Windows, so we run the tests only on Windows -->
<TargetFrameworks>$(NetCoreAppCurrent)-windows</TargetFrameworks>
</PropertyGroup>
<ItemGroup>
<Compile Include="..\**\*.cs" />
<!-- Helpers -->
<Compile Include="$(CommonTestPath)System\Buffers\NativeMemoryManager.cs" Link="Common\System\Buffers\NativeMemoryManager.cs" />
<Compile Include="$(CommonTestPath)System\IO\CallTrackingStream.cs" Link="Common\System\IO\CallTrackingStream.cs" />
<Compile Include="$(CommonTestPath)System\IO\DelegateStream.cs" Link="Common\System\IO\DelegateStream.cs" />
<Compile Include="$(CommonTestPath)System\IO\WrappedMemoryStream.cs" Link="Common\System\IO\WrappedMemoryStream.cs" />
<Compile Include="$(CommonTestPath)System\IO\ConnectedStreams.cs" Link="Common\System\IO\ConnectedStreams.cs" />
<Compile Include="$(CommonPath)System\Net\MultiArrayBuffer.cs" Link="ProductionCode\Common\System\Net\MultiArrayBuffer.cs" />
<Compile Include="$(CommonPath)System\Net\StreamBuffer.cs" Link="ProductionCode\Common\System\Net\StreamBuffer.cs" />
<Compile Include="$(CommonPath)System\Threading\Tasks\TaskToApm.cs" Link="Common\System\Threading\Tasks\TaskToApm.cs" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="$(CommonTestPath)StreamConformanceTests\StreamConformanceTests.csproj" />
</ItemGroup>
</Project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"configProperties": {
"System.IO.UseLegacyFileStream": true
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public static byte[] ReadAllBytes(string path)
{
int n = fs.Read(bytes, index, count);
if (n == 0)
throw Error.GetEndOfFile();
ThrowHelper.ThrowEndOfFileException();
index += n;
count -= n;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -392,11 +392,11 @@
<Compile Include="$(MSBuildThisFileDirectory)System\IO\BinaryReader.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\BinaryWriter.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\BufferedStream.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\BufferedFileStreamStrategy.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\DerivedFileStreamStrategy.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\DirectoryNotFoundException.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\EncodingCache.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\EndOfStreamException.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\Error.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\FileAccess.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\FileAttributes.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\FileLoadException.cs" />
Expand Down Expand Up @@ -1635,14 +1635,17 @@
<Compile Include="$(MSBuildThisFileDirectory)System\Globalization\GlobalizationMode.Windows.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Globalization\HijriCalendar.Win32.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Guid.Windows.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\AsyncWindowsFileStreamStrategy.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\DisableMediaInsertionPrompt.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\DriveInfoInternal.Windows.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\FileStreamHelpers.Windows.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\FileStreamCompletionSource.Win32.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\LegacyFileStreamStrategy.Windows.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\Path.Windows.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\PathHelper.Windows.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\PathInternal.Windows.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\LegacyFileStreamStrategy.Windows.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\SyncWindowsFileStreamStrategy.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\WindowsFileStreamStrategy.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\PasteArguments.Windows.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\Loader\LibraryNameVariation.Windows.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\MemoryFailPoint.Windows.cs" />
Expand Down Expand Up @@ -1895,6 +1898,7 @@
</ItemGroup>
<ItemGroup Condition="'$(TargetsBrowser)' == 'true'">
<Compile Include="$(MSBuildThisFileDirectory)System\AppContext.Browser.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\AppContextConfigHelper.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Environment.Browser.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\DriveInfoInternal.Browser.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\PersistedFiles.Browser.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,20 @@ internal static class AppContextConfigHelper
internal static bool GetBooleanConfig(string configName, bool defaultValue) =>
AppContext.TryGetSwitch(configName, out bool value) ? value : defaultValue;

internal static bool GetBooleanConfig(string switchName, string envVariable)
{
if (!AppContext.TryGetSwitch(switchName, out bool ret))
{
string? switchValue = Environment.GetEnvironmentVariable(envVariable);
if (switchValue != null)
{
ret = bool.IsTrueStringIgnoreCase(switchValue) || switchValue.Equals("1");
}
}

return ret;
}

internal static int GetInt32Config(string configName, int defaultValue, bool allowNegative = true)
{
try
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ internal static partial class GlobalizationMode
internal static bool Invariant { get; } = GetInvariantSwitchValue();

internal static bool UseNls { get; } = !Invariant &&
(GetSwitchValue("System.Globalization.UseNls", "DOTNET_SYSTEM_GLOBALIZATION_USENLS") ||
(AppContextConfigHelper.GetBooleanConfig("System.Globalization.UseNls", "DOTNET_SYSTEM_GLOBALIZATION_USENLS") ||
!LoadIcu());

private static bool LoadIcu()
Expand Down
Loading

0 comments on commit 6ef4b2e

Please sign in to comment.