diff --git a/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs b/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs index ef60a9f223fb28..83b4c87bb64f52 100644 --- a/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs +++ b/src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs @@ -38,15 +38,6 @@ public async Task ReadUsingSingleBuffer(bool asyncOperation, bool asyncHandle) int current = 0; int total = 0; - // From https://docs.microsoft.com/en-us/windows/win32/fileio/file-buffering: - // "File access sizes, including the optional file offset in the OVERLAPPED structure, - // if specified, must be for a number of bytes that is an integer multiple of the volume sector size." - // So if buffer and physical sector size is 4096 and the file size is 4097: - // the read from offset=0 reads 4096 bytes - // the read from offset=4096 reads 1 byte - // the read from offset=4097 THROWS (Invalid argument, offset is not a multiple of sector size!) - // That is why we stop at the first incomplete read (the next one would throw). - // It's possible to get 0 if we are lucky and file size is a multiple of physical sector size. do { current = asyncOperation @@ -57,7 +48,7 @@ public async Task ReadUsingSingleBuffer(bool asyncOperation, bool asyncHandle) total += current; } - while (current == buffer.Memory.Length); + while (current != 0); Assert.Equal(fileSize, total); } @@ -222,6 +213,36 @@ public async Task ReadWriteAsyncUsingEmptyBuffers() await RandomAccess.WriteAsync(handle, Array.Empty>(), 0); } + [Theory] + [MemberData(nameof(AllAsyncSyncCombinations))] + public async Task ReadShouldReturnZeroForEndOfFile(bool asyncOperation, bool asyncHandle) + { + int fileSize = Environment.SystemPageSize + 1; // it MUST NOT be a multiple of it (https://github.com/dotnet/runtime/issues/62851) + string filePath = GetTestFilePath(); + byte[] expected = RandomNumberGenerator.GetBytes(fileSize); + File.WriteAllBytes(filePath, expected); + + using FileStream fileStream = new (filePath, FileMode.Open, FileAccess.Read, FileShare.None, 0, GetFileOptions(asyncHandle)); + using SectorAlignedMemory buffer = SectorAlignedMemory.Allocate(Environment.SystemPageSize); + + int current = 0; + int total = 0; + + do + { + current = asyncOperation + ? await fileStream.ReadAsync(buffer.Memory) + : fileStream.Read(buffer.GetSpan()); + + Assert.True(expected.AsSpan(total, current).SequenceEqual(buffer.GetSpan().Slice(0, current))); + + total += current; + } + while (current != 0); + + Assert.Equal(fileSize, total); + } + // when using FileOptions.Asynchronous we are testing Scatter&Gather APIs on Windows (FILE_FLAG_OVERLAPPED requirement) private static FileOptions GetFileOptions(bool asyncHandle) => (asyncHandle ? FileOptions.Asynchronous : FileOptions.None) | NoBuffering; } diff --git a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs index 32e242e0d8e4c2..6bfd650a0d2fc7 100644 --- a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs @@ -57,6 +57,10 @@ internal bool SupportsRandomAccess internal void EnsureThreadPoolBindingInitialized() { /* nop */ } + internal bool LengthCanBeCached => false; + + internal bool HasCachedFileLength => false; + private static SafeFileHandle Open(string path, Interop.Sys.OpenFlags flags, int mode, Func? createOpenException) { @@ -504,6 +508,13 @@ private bool GetCanSeek() return canSeek == NullableBool.True; } + internal long GetFileLength() + { + int result = Interop.Sys.FStat(this, out Interop.Sys.FileStatus status); + FileStreamHelpers.CheckFileCall(result, Path); + return status.Size; + } + private enum NullableBool { Undefined = 0, diff --git a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs index 3352f0194e73b9..a4ecc686513cb7 100644 --- a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs @@ -4,7 +4,6 @@ using System; using System.Diagnostics; using System.IO; -using System.IO.Strategies; using System.Runtime.InteropServices; using System.Threading; @@ -13,6 +12,8 @@ namespace Microsoft.Win32.SafeHandles public sealed partial class SafeFileHandle : SafeHandleZeroOrMinusOneIsInvalid { internal const FileOptions NoBuffering = (FileOptions)0x20000000; + private long _length = -1; // negative means that hasn't been fetched. + private bool _lengthCanBeCached; // file has been opened for reading and not shared for writing. private volatile FileOptions _fileOptions = (FileOptions)(-1); private volatile int _fileType = -1; @@ -22,10 +23,16 @@ public SafeFileHandle() : base(true) public bool IsAsync => (GetFileOptions() & FileOptions.Asynchronous) != 0; + internal bool IsNoBuffering => (GetFileOptions() & NoBuffering) != 0; + internal bool CanSeek => !IsClosed && GetFileType() == Interop.Kernel32.FileTypes.FILE_TYPE_DISK; internal ThreadPoolBoundHandle? ThreadPoolBinding { get; set; } + internal bool LengthCanBeCached => _lengthCanBeCached; + + internal bool HasCachedFileLength => _lengthCanBeCached && _length >= 0; + internal static unsafe SafeFileHandle Open(string fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize) { using (DisableMediaInsertionPrompt.Create()) @@ -103,6 +110,7 @@ private static unsafe SafeFileHandle CreateFile(string fullPath, FileMode mode, fileHandle._path = fullPath; fileHandle._fileOptions = options; + fileHandle._lengthCanBeCached = (share & FileShare.Write) == 0 && (access & FileAccess.Write) == 0; return fileHandle; } @@ -252,5 +260,34 @@ internal int GetFileType() return fileType; } + + internal unsafe long GetFileLength() + { + if (!_lengthCanBeCached) + { + return GetFileLengthCore(); + } + + // On Windows, when the file is locked for writes we can cache file length + // in memory and avoid subsequent native calls which are expensive. + if (_length < 0) + { + _length = GetFileLengthCore(); + } + + return _length; + + long GetFileLengthCore() + { + Interop.Kernel32.FILE_STANDARD_INFO info; + + if (!Interop.Kernel32.GetFileInformationByHandleEx(this, Interop.Kernel32.FileStandardInfo, &info, (uint)sizeof(Interop.Kernel32.FILE_STANDARD_INFO))) + { + throw Win32Marshal.GetExceptionForLastWin32Error(Path); + } + + return info.EndOfFile; + } + } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/File.cs b/src/libraries/System.Private.CoreLib/src/System/IO/File.cs index aa0bb62be57dc2..2b4c61f1eb0baa 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/File.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/File.cs @@ -256,7 +256,7 @@ public static byte[] ReadAllBytes(string path) using (SafeFileHandle sfh = OpenHandle(path, FileMode.Open, FileAccess.Read, FileShare.Read, options)) { long fileLength = 0; - if (sfh.CanSeek && (fileLength = RandomAccess.GetFileLength(sfh)) > Array.MaxLength) + if (sfh.CanSeek && (fileLength = sfh.GetFileLength()) > Array.MaxLength) { throw new IOException(SR.IO_FileTooLong2GB); } @@ -530,7 +530,7 @@ private static async Task InternalReadAllTextAsync(string path, Encoding SafeFileHandle sfh = OpenHandle(path, FileMode.Open, FileAccess.Read, FileShare.Read, options); long fileLength = 0L; - if (sfh.CanSeek && (fileLength = RandomAccess.GetFileLength(sfh)) > Array.MaxLength) + if (sfh.CanSeek && (fileLength = sfh.GetFileLength()) > Array.MaxLength) { sfh.Dispose(); return Task.FromException(ExceptionDispatchInfo.SetCurrentStackTrace(new IOException(SR.IO_FileTooLong2GB))); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Unix.cs index d922e89c13de5b..d9739e3082dfdb 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Unix.cs @@ -19,13 +19,6 @@ public static partial class RandomAccess // that get stackalloced in the Linux kernel. private const int IovStackThreshold = 8; - internal static long GetFileLength(SafeFileHandle handle) - { - int result = Interop.Sys.FStat(handle, out Interop.Sys.FileStatus status); - FileStreamHelpers.CheckFileCall(result, handle.Path); - return status.Size; - } - internal static unsafe int ReadAtOffset(SafeFileHandle handle, Span buffer, long fileOffset) { fixed (byte* bufPtr = &MemoryMarshal.GetReference(buffer)) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs index 348b0bf4f15a52..4e79f02de40816 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs @@ -18,18 +18,6 @@ public static partial class RandomAccess { private static readonly IOCompletionCallback s_callback = AllocateCallback(); - internal static unsafe long GetFileLength(SafeFileHandle handle) - { - Interop.Kernel32.FILE_STANDARD_INFO info; - - if (!Interop.Kernel32.GetFileInformationByHandleEx(handle, Interop.Kernel32.FileStandardInfo, &info, (uint)sizeof(Interop.Kernel32.FILE_STANDARD_INFO))) - { - throw Win32Marshal.GetExceptionForLastWin32Error(handle.Path); - } - - return info.EndOfFile; - } - internal static unsafe int ReadAtOffset(SafeFileHandle handle, Span buffer, long fileOffset) { if (handle.IsAsync) @@ -53,8 +41,8 @@ internal static unsafe int ReadAtOffset(SafeFileHandle handle, Span buffer // "If lpOverlapped is not NULL, then when a synchronous read operation reaches the end of a file, // ReadFile returns FALSE and GetLastError returns ERROR_HANDLE_EOF" return numBytesRead; - case Interop.Errors.ERROR_BROKEN_PIPE: - // For pipes, ERROR_BROKEN_PIPE is the normal end of the pipe. + case Interop.Errors.ERROR_BROKEN_PIPE: // For pipes, ERROR_BROKEN_PIPE is the normal end of the pipe. + case Interop.Errors.ERROR_INVALID_PARAMETER when IsEndOfFileForNoBuffering(handle, fileOffset): return 0; default: throw Win32Marshal.GetExceptionForWin32Error(errorCode, handle.Path); @@ -100,6 +88,7 @@ private static unsafe int ReadSyncUsingAsyncHandle(SafeFileHandle handle, Span fileHandle.IsNoBuffering && fileHandle.CanSeek && fileOffset >= fileHandle.GetFileLength(); + // We need to store the reference count (see the comment in FreeNativeOverlappedIfItIsSafe) and an EventHandle to signal the completion. // We could keep these two things separate, but since ManualResetEvent is sealed and we want to avoid any extra allocations, this type has been created. // It's basically ManualResetEvent with reference count. diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.cs index 04b59ae472a769..5c4ede7ff408b5 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.cs @@ -3,7 +3,6 @@ using System.Collections.Generic; using System.IO.Strategies; -using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; using Microsoft.Win32.SafeHandles; @@ -25,7 +24,7 @@ public static long GetLength(SafeFileHandle handle) { ValidateInput(handle, fileOffset: 0); - return GetFileLength(handle); + return handle.GetFileLength(); } /// diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/OSFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/OSFileStreamStrategy.cs index 0386dfa78d5631..662945c0941e92 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/OSFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/OSFileStreamStrategy.cs @@ -4,7 +4,6 @@ using System.Diagnostics; using System.Threading; using System.Threading.Tasks; -using System.Threading.Tasks.Sources; using Microsoft.Win32.SafeHandles; namespace System.IO.Strategies @@ -16,9 +15,7 @@ internal abstract class OSFileStreamStrategy : FileStreamStrategy private readonly FileAccess _access; // What file was opened for. protected long _filePosition; - private long _length = -1; // negative means that hasn't been fetched. private long _appendStart; // When appending, prevent overwriting file. - private bool _lengthCanBeCached; // SafeFileHandle hasn't been exposed, file has been opened for reading and not shared for writing. internal OSFileStreamStrategy(SafeFileHandle handle, FileAccess access) { @@ -45,7 +42,6 @@ internal OSFileStreamStrategy(string path, FileMode mode, FileAccess access, Fil string fullPath = Path.GetFullPath(path); _access = access; - _lengthCanBeCached = (share & FileShare.Write) == 0 && (access & FileAccess.Write) == 0; _fileHandle = SafeFileHandle.Open(fullPath, mode, access, share, options, preallocationSize); @@ -78,34 +74,13 @@ internal OSFileStreamStrategy(string path, FileMode mode, FileAccess access, Fil public sealed override bool CanWrite => !_fileHandle.IsClosed && (_access & FileAccess.Write) != 0; - public unsafe sealed override long Length - { - get - { - if (!LengthCachingSupported) - { - return RandomAccess.GetFileLength(_fileHandle); - } - - // On Windows, when the file is locked for writes we can cache file length - // in memory and avoid subsequent native calls which are expensive. - - if (_length < 0) - { - _length = RandomAccess.GetFileLength(_fileHandle); - } - - return _length; - } - } + public unsafe sealed override long Length => _fileHandle.GetFileLength(); // in case of concurrent incomplete reads, there can be multiple threads trying to update the position // at the same time. That is why we are using Interlocked here. internal void OnIncompleteOperation(int expectedBytesTransferred, int actualBytesTransferred) => Interlocked.Add(ref _filePosition, actualBytesTransferred - expectedBytesTransferred); - private bool LengthCachingSupported => OperatingSystem.IsWindows() && _lengthCanBeCached; - /// Gets or sets the position within the current stream public sealed override long Position { @@ -129,9 +104,6 @@ internal sealed override SafeFileHandle SafeFileHandle FileStreamHelpers.Seek(_fileHandle, _filePosition, SeekOrigin.Begin); } - _lengthCanBeCached = false; - _length = -1; // invalidate cached length - return _fileHandle; } } @@ -224,10 +196,7 @@ protected unsafe void SetLengthCore(long value) Debug.Assert(value >= 0, "value >= 0"); FileStreamHelpers.SetFileLength(_fileHandle, value); - if (LengthCachingSupported) - { - _length = value; - } + Debug.Assert(!_fileHandle.LengthCanBeCached, "If length can be cached (file opened for reading, not shared for writing), it should be impossible to modify file length"); if (_filePosition > value) { @@ -314,7 +283,7 @@ public sealed override ValueTask ReadAsync(Memory destination, Cancel return RandomAccess.ReadAtOffsetAsync(_fileHandle, destination, fileOffset: -1, cancellationToken); } - if (LengthCachingSupported && _length >= 0 && Volatile.Read(ref _filePosition) >= _length) + if (_fileHandle.HasCachedFileLength && Volatile.Read(ref _filePosition) >= _fileHandle.GetFileLength()) { // We know for sure that the file length can be safely cached and it has already been obtained. // If we have reached EOF we just return here and avoid a sys-call.