From 179d7be7b6188134fdb9a44650ef1bb367883ad7 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 4 Mar 2021 09:19:30 +0100 Subject: [PATCH 01/11] don't verify OS handle position --- .../tests/FileStream/SafeFileHandle.cs | 4 +- .../AsyncWindowsFileStreamStrategy.cs | 17 +----- .../SyncWindowsFileStreamStrategy.cs | 6 -- .../Strategies/WindowsFileStreamStrategy.cs | 58 ++----------------- 4 files changed, 8 insertions(+), 77 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs index 57ce00dea3d21..c726519b01375 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs @@ -66,13 +66,13 @@ public void AccessFlushesFileClosesHandle() } } - [Fact] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsLegacyFileStreamEnabled))] public async Task ThrowWhenHandlePositionIsChanged_sync() { await ThrowWhenHandlePositionIsChanged(useAsync: false); } - [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported), nameof(PlatformDetection.IsLegacyFileStreamEnabled))] public async Task ThrowWhenHandlePositionIsChanged_async() { await ThrowWhenHandlePositionIsChanged(useAsync: true); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs index b02366ecfd13d..953a2c0c59287 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs @@ -98,7 +98,6 @@ protected override void OnInit() if (_fileHandle.ThreadPoolBinding == null) { // We should close the handle so that the handle is not open until SafeFileHandle GC - Debug.Assert(!_exposedHandle, "Are we closing handle that we exposed/not own, how?"); _fileHandle.Dispose(); } } @@ -146,10 +145,7 @@ private unsafe Task ReadAsyncInternal(Memory destination, Cancellatio { long len = Length; - // Make sure we are reading from the position that we think we are - VerifyOSHandlePosition(); - - if (destination.Length > len - _filePosition) + if (_filePosition + destination.Length > len) { if (_filePosition <= len) { @@ -274,9 +270,6 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, Cancella // Make sure we set the length of the file appropriately. long len = Length; - // Make sure we are writing to the position that we think we are - VerifyOSHandlePosition(); - if (_filePosition + source.Length > len) { SetLengthCore(_filePosition + source.Length); @@ -382,16 +375,10 @@ private async Task AsyncModeCopyToAsync(Stream destination, int bufferSize, Canc Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); Debug.Assert(CanRead, "_parent.CanRead"); - bool canSeek = CanSeek; - if (canSeek) - { - VerifyOSHandlePosition(); - } - try { await FileStreamHelpers - .AsyncModeCopyToAsync(_fileHandle, _path, canSeek, _filePosition, destination, bufferSize, cancellationToken) + .AsyncModeCopyToAsync(_fileHandle, _path, CanSeek, _filePosition, destination, bufferSize, cancellationToken) .ConfigureAwait(false); } finally diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/SyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/SyncWindowsFileStreamStrategy.cs index bcd9c67e7f8ae..8b2fb2a59bd91 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/SyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/SyncWindowsFileStreamStrategy.cs @@ -104,9 +104,6 @@ private unsafe int ReadSpan(Span destination) Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); - // Make sure we are reading from the right spot - VerifyOSHandlePosition(); - int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination, null, out int errorCode); if (r == -1) @@ -139,9 +136,6 @@ private unsafe void WriteSpan(ReadOnlySpan source) Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); - // Make sure we are writing to the position that we think we are - VerifyOSHandlePosition(); - int r = FileStreamHelpers.WriteFileNative(_fileHandle, source, null, out int errorCode); if (r == -1) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs index d238bc23e4c4e..6aca823be017d 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs @@ -25,13 +25,10 @@ internal abstract class WindowsFileStreamStrategy : FileStreamStrategy private readonly bool _isPipe; // Whether to disable async buffering code. protected long _filePosition; - protected bool _exposedHandle; // Whether the file stream's handle has been exposed. private long _appendStart; // When appending, prevent overwriting file. internal WindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access) { - _exposedHandle = true; - InitFromHandle(handle, access, out _canSeek, out _isPipe); // Note: Cleaner to set the following fields in ValidateAndInitFromHandle, @@ -79,16 +76,8 @@ internal WindowsFileStreamStrategy(string path, FileMode mode, FileAccess access /// Gets or sets the position within the current stream public override long Position { - get - { - VerifyOSHandlePosition(); - - return _filePosition; - } - set - { - Seek(value, SeekOrigin.Begin); - } + get => _filePosition; + set => Seek(value, SeekOrigin.Begin); } internal sealed override string Name => _path ?? SR.IO_UnknownFileName; @@ -96,16 +85,8 @@ public override long Position internal sealed override bool IsClosed => _fileHandle.IsClosed; internal sealed override bool IsPipe => _isPipe; - - internal sealed override SafeFileHandle SafeFileHandle - { - get - { - // Flushing is the responsibility of BufferedFileStreamStrategy - _exposedHandle = true; - return _fileHandle; - } - } + // Flushing is the responsibility of BufferedFileStreamStrategy + internal sealed override SafeFileHandle SafeFileHandle => _fileHandle; // ReadByte and WriteByte methods are used only when the user has disabled buffering on purpose // their performance is going to be horrible @@ -165,9 +146,6 @@ public sealed override long Seek(long offset, SeekOrigin origin) if (_fileHandle.IsClosed) ThrowHelper.ThrowObjectDisposedException_FileClosed(); if (!CanSeek) ThrowHelper.ThrowNotSupportedException_UnseekableStream(); - // Verify that internal position is in sync with the handle - VerifyOSHandlePosition(); - long oldPos = _filePosition; long pos = SeekCore(_fileHandle, offset, origin); @@ -261,7 +239,6 @@ public sealed override void SetLength(long value) protected unsafe void SetLengthCore(long value) { Debug.Assert(value >= 0, "value >= 0"); - VerifyOSHandlePosition(); FileStreamHelpers.SetFileLength(_fileHandle, _path, value); @@ -270,32 +247,5 @@ protected unsafe void SetLengthCore(long value) SeekCore(_fileHandle, 0, SeekOrigin.End); } } - - /// - /// Verify that the actual position of the OS's handle equals what we expect it to. - /// This will fail if someone else moved the UnixFileStream's handle or if - /// our position updating code is incorrect. - /// - [MethodImpl(MethodImplOptions.AggressiveInlining)] - protected void VerifyOSHandlePosition() - { - bool verifyPosition = _exposedHandle; // in release, only verify if we've given out the handle such that someone else could be manipulating it -#if DEBUG - verifyPosition = true; // in debug, always make sure our position matches what the OS says it should be -#endif - if (verifyPosition && CanSeek) - { - long oldPos = _filePosition; // SeekCore will override the current _position, so save it now - long curPos = SeekCore(_fileHandle, 0, SeekOrigin.Current); - if (oldPos != curPos) - { - // For reads, this is non-fatal but we still could have returned corrupted - // data in some cases, so discard the internal buffer. For writes, - // this is a problem; discard the buffer and error out. - - throw new IOException(SR.IO_FileStreamHandlePosition); - } - } - } } } From a0dfd18dd8b0264451f6a6729142c6e07a61a45e Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 4 Mar 2021 10:22:05 +0100 Subject: [PATCH 02/11] track the file offset in memory, don't use expensive sys calls to synchronize it --- ...op.ReadFile_SafeHandle_NativeOverlapped.cs | 8 +++ ...p.WriteFile_SafeHandle_NativeOverlapped.cs | 3 ++ .../AsyncWindowsFileStreamStrategy.cs | 44 ++++++++-------- .../Strategies/FileStreamHelpers.Windows.cs | 36 ++++++++++--- .../LegacyFileStreamStrategy.Windows.cs | 4 +- .../SyncWindowsFileStreamStrategy.cs | 16 +++++- .../Strategies/WindowsFileStreamStrategy.cs | 50 +++++++++++-------- 7 files changed, 106 insertions(+), 55 deletions(-) diff --git a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ReadFile_SafeHandle_NativeOverlapped.cs b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ReadFile_SafeHandle_NativeOverlapped.cs index 8419932249299..c6fb2c4311cf3 100644 --- a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ReadFile_SafeHandle_NativeOverlapped.cs +++ b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ReadFile_SafeHandle_NativeOverlapped.cs @@ -16,5 +16,13 @@ internal static extern unsafe int ReadFile( int numBytesToRead, IntPtr numBytesRead_mustBeZero, NativeOverlapped* overlapped); + + [DllImport(Libraries.Kernel32, SetLastError = true)] + internal static extern unsafe int ReadFile( + SafeHandle handle, + byte* bytes, + int numBytesToRead, + out int numBytesRead, + NativeOverlapped* overlapped); } } diff --git a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.WriteFile_SafeHandle_NativeOverlapped.cs b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.WriteFile_SafeHandle_NativeOverlapped.cs index cca93e0b3ac89..3eaff9ca07626 100644 --- a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.WriteFile_SafeHandle_NativeOverlapped.cs +++ b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.WriteFile_SafeHandle_NativeOverlapped.cs @@ -17,5 +17,8 @@ internal static partial class Kernel32 // and pass in an address for the numBytesRead parameter. [DllImport(Libraries.Kernel32, SetLastError = true)] internal static extern unsafe int WriteFile(SafeHandle handle, byte* bytes, int numBytesToWrite, IntPtr numBytesWritten_mustBeZero, NativeOverlapped* lpOverlapped); + + [DllImport(Libraries.Kernel32, SetLastError = true)] + internal static extern unsafe int WriteFile(SafeHandle handle, byte* bytes, int numBytesToWrite, out int numBytesWritten, NativeOverlapped* lpOverlapped); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs index 953a2c0c59287..b1d4197bebf2d 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs @@ -141,15 +141,16 @@ private unsafe Task ReadAsyncInternal(Memory destination, Cancellatio NativeOverlapped* intOverlapped = completionSource.Overlapped; // Calculate position in the file we should be at after the read is done + long positionBefore = _filePosition; if (CanSeek) { long len = Length; - if (_filePosition + destination.Length > len) + if (positionBefore + destination.Length > len) { - if (_filePosition <= len) + if (positionBefore <= len) { - destination = destination.Slice(0, (int)(len - _filePosition)); + destination = destination.Slice(0, (int)(len - positionBefore)); } else { @@ -159,23 +160,17 @@ private unsafe Task ReadAsyncInternal(Memory destination, Cancellatio // Now set the position to read from in the NativeOverlapped struct // For pipes, we should leave the offset fields set to 0. - intOverlapped->OffsetLow = unchecked((int)_filePosition); - intOverlapped->OffsetHigh = (int)(_filePosition >> 32); + intOverlapped->OffsetLow = unchecked((int)positionBefore); + intOverlapped->OffsetHigh = (int)(positionBefore >> 32); // When using overlapped IO, the OS is not supposed to // touch the file pointer location at all. We will adjust it - // ourselves. This isn't threadsafe. - - // WriteFile should not update the file pointer when writing - // in overlapped mode, according to MSDN. But it does update - // the file pointer when writing to a UNC path! - // So changed the code below to seek to an absolute - // location, not a relative one. ReadFile seems consistent though. - SeekCore(_fileHandle, destination.Length, SeekOrigin.Current); + // ourselves, but only in memory. This isn't threadsafe. + _filePosition += destination.Length; } // queue an async ReadFile operation and pass in a packed overlapped - int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination.Span, intOverlapped, out int errorCode); + int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination.Span, false, intOverlapped, out int errorCode); // ReadFile, the OS version, will return 0 on failure. But // my ReadFileNative wrapper returns -1. My wrapper will return @@ -204,7 +199,7 @@ private unsafe Task ReadAsyncInternal(Memory destination, Cancellatio { if (!_fileHandle.IsClosed && CanSeek) // Update Position - It could be anywhere. { - SeekCore(_fileHandle, 0, SeekOrigin.Current); + _filePosition = positionBefore; } completionSource.ReleaseNativeResource(); @@ -265,29 +260,30 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, Cancella FileStreamCompletionSource completionSource = FileStreamCompletionSource.Create(this, _preallocatedOverlapped, 0, source); NativeOverlapped* intOverlapped = completionSource.Overlapped; + long positionBefore = _filePosition; if (CanSeek) { // Make sure we set the length of the file appropriately. long len = Length; - if (_filePosition + source.Length > len) + if (positionBefore + source.Length > len) { - SetLengthCore(_filePosition + source.Length); + SetLengthCore(positionBefore + source.Length); } // Now set the position to read from in the NativeOverlapped struct // For pipes, we should leave the offset fields set to 0. - intOverlapped->OffsetLow = (int)_filePosition; - intOverlapped->OffsetHigh = (int)(_filePosition >> 32); + intOverlapped->OffsetLow = (int)positionBefore; + intOverlapped->OffsetHigh = (int)(positionBefore >> 32); // When using overlapped IO, the OS is not supposed to // touch the file pointer location at all. We will adjust it - // ourselves. This isn't threadsafe. - SeekCore(_fileHandle, source.Length, SeekOrigin.Current); + // ourselves, but only in memory. This isn't threadsafe. + _filePosition += source.Length; } // queue an async WriteFile operation and pass in a packed overlapped - int r = FileStreamHelpers.WriteFileNative(_fileHandle, source.Span, intOverlapped, out int errorCode); + int r = FileStreamHelpers.WriteFileNative(_fileHandle, source.Span, false, intOverlapped, out int errorCode); // WriteFile, the OS version, will return 0 on failure. But // my WriteFileNative wrapper returns -1. My wrapper will return @@ -313,7 +309,7 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, Cancella { if (!_fileHandle.IsClosed && CanSeek) // Update Position - It could be anywhere. { - SeekCore(_fileHandle, 0, SeekOrigin.Current); + _filePosition = positionBefore; } completionSource.ReleaseNativeResource(); @@ -386,7 +382,7 @@ await FileStreamHelpers // Make sure the stream's current position reflects where we ended up if (!_fileHandle.IsClosed && canSeek) { - SeekCore(_fileHandle, 0, SeekOrigin.End); + _filePosition = Length; } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs index f565d1f1e0f3b..8d56567233ca6 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs @@ -333,7 +333,7 @@ internal static unsafe void SetFileLength(SafeFileHandle handle, string? path, l } // __ConsoleStream also uses this code. - internal static unsafe int ReadFileNative(SafeFileHandle handle, Span bytes, NativeOverlapped* overlapped, out int errorCode) + internal static unsafe int ReadFileNative(SafeFileHandle handle, Span bytes, bool syncUsingOverlapped, NativeOverlapped* overlapped, out int errorCode) { Debug.Assert(handle != null, "handle != null"); @@ -343,13 +343,24 @@ internal static unsafe int ReadFileNative(SafeFileHandle handle, Span byte fixed (byte* p = &MemoryMarshal.GetReference(bytes)) { r = overlapped != null ? - Interop.Kernel32.ReadFile(handle, p, bytes.Length, IntPtr.Zero, overlapped) : - Interop.Kernel32.ReadFile(handle, p, bytes.Length, out numBytesRead, IntPtr.Zero); + (syncUsingOverlapped + ? Interop.Kernel32.ReadFile(handle, p, bytes.Length, out numBytesRead, overlapped) + : Interop.Kernel32.ReadFile(handle, p, bytes.Length, IntPtr.Zero, overlapped)) + : Interop.Kernel32.ReadFile(handle, p, bytes.Length, out numBytesRead, IntPtr.Zero); } if (r == 0) { errorCode = GetLastWin32ErrorAndDisposeHandleIfInvalid(handle); + + if (syncUsingOverlapped && errorCode == Interop.Errors.ERROR_HANDLE_EOF) + { + // https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-readfile#synchronization-and-file-position : + // "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; + } + return -1; } else @@ -359,7 +370,7 @@ internal static unsafe int ReadFileNative(SafeFileHandle handle, Span byte } } - internal static unsafe int WriteFileNative(SafeFileHandle handle, ReadOnlySpan buffer, NativeOverlapped* overlapped, out int errorCode) + internal static unsafe int WriteFileNative(SafeFileHandle handle, ReadOnlySpan buffer, bool syncUsingOverlapped, NativeOverlapped* overlapped, out int errorCode) { Debug.Assert(handle != null, "handle != null"); @@ -369,13 +380,24 @@ internal static unsafe int WriteFileNative(SafeFileHandle handle, ReadOnlySpan= 0; + synchronousSuccess = ReadFileNative(handle, copyBuffer, false, readAwaitable._nativeOverlapped, out errorCode) >= 0; } // If the operation did not synchronously succeed, it either failed or initiated the asynchronous operation. diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/LegacyFileStreamStrategy.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/LegacyFileStreamStrategy.Windows.cs index 8890bf3053474..e562f0c2e2151 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/LegacyFileStreamStrategy.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/LegacyFileStreamStrategy.Windows.cs @@ -1079,14 +1079,14 @@ private unsafe int ReadFileNative(SafeFileHandle handle, Span bytes, Nativ { Debug.Assert((_useAsyncIO && overlapped != null) || (!_useAsyncIO && overlapped == null), "Async IO and overlapped parameters inconsistent in call to ReadFileNative."); - return FileStreamHelpers.ReadFileNative(handle, bytes, overlapped, out errorCode); + return FileStreamHelpers.ReadFileNative(handle, bytes, false, overlapped, out errorCode); } private unsafe int WriteFileNative(SafeFileHandle handle, ReadOnlySpan buffer, NativeOverlapped* overlapped, out int errorCode) { Debug.Assert((_useAsyncIO && overlapped != null) || (!_useAsyncIO && overlapped == null), "Async IO and overlapped parameters inconsistent in call to WriteFileNative."); - return FileStreamHelpers.WriteFileNative(handle, buffer, overlapped, out errorCode); + return FileStreamHelpers.WriteFileNative(handle, buffer, false, overlapped, out errorCode); } public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/SyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/SyncWindowsFileStreamStrategy.cs index 8b2fb2a59bd91..046683a9a2503 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/SyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/SyncWindowsFileStreamStrategy.cs @@ -104,7 +104,8 @@ private unsafe int ReadSpan(Span destination) Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); - int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination, null, out int errorCode); + NativeOverlapped nativeOverlapped = GetNativeOverlappedForCurrentPosition(); + int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination, true, &nativeOverlapped, out int errorCode); if (r == -1) { @@ -136,7 +137,8 @@ private unsafe void WriteSpan(ReadOnlySpan source) Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); - int r = FileStreamHelpers.WriteFileNative(_fileHandle, source, null, out int errorCode); + NativeOverlapped nativeOverlapped = GetNativeOverlappedForCurrentPosition(); + int r = FileStreamHelpers.WriteFileNative(_fileHandle, source, true, &nativeOverlapped, out int errorCode); if (r == -1) { @@ -159,5 +161,15 @@ private unsafe void WriteSpan(ReadOnlySpan source) _filePosition += r; return; } + + private NativeOverlapped GetNativeOverlappedForCurrentPosition() + { + NativeOverlapped nativeOverlapped = default; + // For pipes the offsets are ignored by the OS + nativeOverlapped.OffsetLow = unchecked((int)_filePosition); + nativeOverlapped.OffsetHigh = (int)(_filePosition >> 32); + + return nativeOverlapped; + } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs index 6aca823be017d..6a0f00f8e630f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs @@ -77,7 +77,7 @@ internal WindowsFileStreamStrategy(string path, FileMode mode, FileAccess access public override long Position { get => _filePosition; - set => Seek(value, SeekOrigin.Begin); + set => _filePosition = value; } internal sealed override string Name => _path ?? SR.IO_UnknownFileName; @@ -86,6 +86,8 @@ public override long Position internal sealed override bool IsPipe => _isPipe; // Flushing is the responsibility of BufferedFileStreamStrategy + // TODO: we might consider calling FileStreamHelpers.Seek(handle, _path, _filePosition, SeekOrigin.Begin); + // here to set the actual file offset internal sealed override SafeFileHandle SafeFileHandle => _fileHandle; // ReadByte and WriteByte methods are used only when the user has disabled buffering on purpose @@ -147,29 +149,33 @@ public sealed override long Seek(long offset, SeekOrigin origin) if (!CanSeek) ThrowHelper.ThrowNotSupportedException_UnseekableStream(); long oldPos = _filePosition; - long pos = SeekCore(_fileHandle, offset, origin); + long pos = origin switch + { + SeekOrigin.Begin => offset, + SeekOrigin.End => FileStreamHelpers.GetFileLength(_fileHandle, _path) + offset, + _ => _filePosition + offset // SeekOrigin.Current + }; + + if (pos >= 0) + { + _filePosition = pos; + } + else + { + // keep throwing the same exception we did when seek was causing actual offset change + throw Win32Marshal.GetExceptionForWin32Error(Interop.Errors.ERROR_INVALID_PARAMETER); + } - // Prevent users from overwriting data in a file that was opened in - // append mode. + // Prevent users from overwriting data in a file that was opened in append mode. if (_appendStart != -1 && pos < _appendStart) { - SeekCore(_fileHandle, oldPos, SeekOrigin.Begin); + _filePosition = oldPos; throw new IOException(SR.IO_SeekAppendOverwrite); } return pos; } - // This doesn't do argument checking. Necessary for SetLength, which must - // set the file pointer beyond the end of the file. This will update the - // internal position - protected long SeekCore(SafeFileHandle fileHandle, long offset, SeekOrigin origin, bool closeInvalidHandle = false) - { - Debug.Assert(!fileHandle.IsClosed && _canSeek, "!fileHandle.IsClosed && _canSeek"); - - return _filePosition = FileStreamHelpers.Seek(fileHandle, _path, offset, origin, closeInvalidHandle); - } - internal sealed override void Lock(long position, long length) => FileStreamHelpers.Lock(_fileHandle, _path, position, length); internal sealed override void Unlock(long position, long length) => FileStreamHelpers.Unlock(_fileHandle, _path, position, length); @@ -187,7 +193,7 @@ private void Init(FileMode mode, string originalPath) // For Append mode... if (mode == FileMode.Append) { - _appendStart = SeekCore(_fileHandle, 0, SeekOrigin.End); + _appendStart = _filePosition = Length; } else { @@ -221,9 +227,15 @@ private void InitFromHandleImpl(SafeFileHandle handle, out bool canSeek, out boo OnInitFromHandle(handle); if (_canSeek) - SeekCore(handle, 0, SeekOrigin.Current); + { + // given strategy was created out of existing handle, so we have to perform + // a syscall to get the current handle offset + _filePosition = FileStreamHelpers.Seek(handle, _path, 0, SeekOrigin.Current); + } else + { _filePosition = 0; + } } public sealed override void SetLength(long value) @@ -234,8 +246,6 @@ public sealed override void SetLength(long value) SetLengthCore(value); } - // We absolutely need this method broken out so that WriteInternalCoreAsync can call - // a method without having to go through buffering code that might call FlushWrite. protected unsafe void SetLengthCore(long value) { Debug.Assert(value >= 0, "value >= 0"); @@ -244,7 +254,7 @@ protected unsafe void SetLengthCore(long value) if (_filePosition > value) { - SeekCore(_fileHandle, 0, SeekOrigin.End); + _filePosition = value; } } } From cbebeb1af5a02b456d940b2d5e764afe0fc80018 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 5 Mar 2021 11:17:44 +0100 Subject: [PATCH 03/11] there is no need to set the Length since we are now tracking the offset in memory --- .../IO/Strategies/AsyncWindowsFileStreamStrategy.cs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs index b1d4197bebf2d..dceef081d39ec 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs @@ -263,14 +263,6 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, Cancella long positionBefore = _filePosition; if (CanSeek) { - // Make sure we set the length of the file appropriately. - long len = Length; - - if (positionBefore + source.Length > len) - { - SetLengthCore(positionBefore + source.Length); - } - // Now set the position to read from in the NativeOverlapped struct // For pipes, we should leave the offset fields set to 0. intOverlapped->OffsetLow = (int)positionBefore; From f6680352ecd8d6accfa819dcf7c6892cda856d7b Mon Sep 17 00:00:00 2001 From: David Cantu Date: Mon, 15 Mar 2021 01:26:47 -0700 Subject: [PATCH 04/11] Cache GetFileInformationByHandleEx (Length) when FileShare does not allow other processes to modify it --- .../src/System/IO/FileStream.cs | 2 +- .../AsyncWindowsFileStreamStrategy.cs | 3 ++- .../SyncWindowsFileStreamStrategy.cs | 2 +- .../Strategies/WindowsFileStreamStrategy.cs | 25 ++++++++++++++++++- 4 files changed, 28 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs index 439f00c0c5c8f..5b8c213b427fe 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs @@ -13,7 +13,7 @@ namespace System.IO public class FileStream : Stream { internal const int DefaultBufferSize = 4096; - private const FileShare DefaultShare = FileShare.Read; + internal const FileShare DefaultShare = FileShare.Read; private const bool DefaultIsAsync = false; /// Caches whether Serialization Guard has been disabled for file writes diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs index dceef081d39ec..1209f5eedd1c2 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs @@ -272,6 +272,7 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, Cancella // touch the file pointer location at all. We will adjust it // ourselves, but only in memory. This isn't threadsafe. _filePosition += source.Length; + UpdateLengthOnChangePosition(); } // queue an async WriteFile operation and pass in a packed overlapped @@ -372,7 +373,7 @@ await FileStreamHelpers finally { // Make sure the stream's current position reflects where we ended up - if (!_fileHandle.IsClosed && canSeek) + if (!_fileHandle.IsClosed && CanSeek) { _filePosition = Length; } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/SyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/SyncWindowsFileStreamStrategy.cs index 046683a9a2503..4545e1dff0b15 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/SyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/SyncWindowsFileStreamStrategy.cs @@ -159,7 +159,7 @@ private unsafe void WriteSpan(ReadOnlySpan source) } Debug.Assert(r >= 0, "FileStream's WriteCore is likely broken."); _filePosition += r; - return; + UpdateLengthOnChangePosition(); } private NativeOverlapped GetNativeOverlappedForCurrentPosition() diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs index 6a0f00f8e630f..52d8eda880e50 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs @@ -21,11 +21,13 @@ internal abstract class WindowsFileStreamStrategy : FileStreamStrategy protected readonly SafeFileHandle _fileHandle; // only ever null if ctor throws protected readonly string? _path; // The path to the opened file. private readonly FileAccess _access; // What file was opened for. + private readonly FileShare _share; private readonly bool _canSeek; // Whether can seek (file) or not (pipe). private readonly bool _isPipe; // Whether to disable async buffering code. protected long _filePosition; private long _appendStart; // When appending, prevent overwriting file. + private long? _length; internal WindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access) { @@ -34,6 +36,7 @@ internal WindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access) // Note: Cleaner to set the following fields in ValidateAndInitFromHandle, // but we can't as they're readonly. _access = access; + _share = FileStream.DefaultShare; // As the handle was passed in, we must set the handle field at the very end to // avoid the finalizer closing the handle when we throw errors. @@ -46,6 +49,7 @@ internal WindowsFileStreamStrategy(string path, FileMode mode, FileAccess access _path = fullPath; _access = access; + _share = share; _fileHandle = FileStreamHelpers.OpenHandle(fullPath, mode, access, share, options); @@ -71,7 +75,25 @@ internal WindowsFileStreamStrategy(string path, FileMode mode, FileAccess access public sealed override bool CanWrite => !_fileHandle.IsClosed && (_access & FileAccess.Write) != 0; - public unsafe sealed override long Length => FileStreamHelpers.GetFileLength(_fileHandle, _path); + public unsafe sealed override long Length => _share > FileShare.Read ? + FileStreamHelpers.GetFileLength(_fileHandle, _path) : + _length ??= FileStreamHelpers.GetFileLength(_fileHandle, _path); + + protected void UpdateLengthOnChangePosition() + { + // Do not update the cached length if the file can be written somewhere else + // or if the length has not been queried. + if (_share > FileShare.Read || _length is null) + { + Debug.Assert(_length is null); + return; + } + + if (_filePosition > _length) + { + _length = _filePosition; + } + } /// Gets or sets the position within the current stream public override long Position @@ -251,6 +273,7 @@ protected unsafe void SetLengthCore(long value) Debug.Assert(value >= 0, "value >= 0"); FileStreamHelpers.SetFileLength(_fileHandle, _path, value); + _length = value; if (_filePosition > value) { From 64e5174524b8a6d96c0b0e101977638ac13fffc5 Mon Sep 17 00:00:00 2001 From: David Cantu Date: Sun, 21 Mar 2021 21:16:55 -0700 Subject: [PATCH 05/11] Update offset before get_SafeFileHandle returns --- .../IO/Strategies/WindowsFileStreamStrategy.cs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs index 52d8eda880e50..9aa9d9c0bbf3e 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs @@ -108,9 +108,15 @@ public override long Position internal sealed override bool IsPipe => _isPipe; // Flushing is the responsibility of BufferedFileStreamStrategy - // TODO: we might consider calling FileStreamHelpers.Seek(handle, _path, _filePosition, SeekOrigin.Begin); - // here to set the actual file offset - internal sealed override SafeFileHandle SafeFileHandle => _fileHandle; + internal sealed override SafeFileHandle SafeFileHandle + { + get + { + // Update the file offset in the handle before exposing it. + FileStreamHelpers.Seek(_fileHandle, _path, _filePosition, SeekOrigin.Begin); + return _fileHandle; + } + } // ReadByte and WriteByte methods are used only when the user has disabled buffering on purpose // their performance is going to be horrible From 3843c637744e6e342730c27d96a844e80fcead98 Mon Sep 17 00:00:00 2001 From: David Cantu Date: Sun, 21 Mar 2021 21:47:02 -0700 Subject: [PATCH 06/11] Address suggestions --- .../src/System/IO/FileStream.cs | 6 +++--- .../Strategies/AsyncWindowsFileStreamStrategy.cs | 4 ++-- .../IO/Strategies/FileStreamHelpers.Windows.cs | 6 +++--- .../src/System/IO/Strategies/FileStreamHelpers.cs | 4 ++-- .../IO/Strategies/SyncWindowsFileStreamStrategy.cs | 2 +- .../IO/Strategies/WindowsFileStreamStrategy.cs | 14 +++++++------- 6 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs index 5b8c213b427fe..eb9d98750f758 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs @@ -13,7 +13,7 @@ namespace System.IO public class FileStream : Stream { internal const int DefaultBufferSize = 4096; - internal const FileShare DefaultShare = FileShare.Read; + private const FileShare DefaultShare = FileShare.Read; private const bool DefaultIsAsync = false; /// Caches whether Serialization Guard has been disabled for file writes @@ -47,7 +47,7 @@ public FileStream(IntPtr handle, FileAccess access, bool ownsHandle, int bufferS { ValidateHandle(safeHandle, access, bufferSize, isAsync); - _strategy = FileStreamHelpers.ChooseStrategy(this, safeHandle, access, bufferSize, isAsync); + _strategy = FileStreamHelpers.ChooseStrategy(this, safeHandle, access, DefaultShare, bufferSize, isAsync); } catch { @@ -101,7 +101,7 @@ public FileStream(SafeFileHandle handle, FileAccess access, int bufferSize, bool { ValidateHandle(handle, access, bufferSize, isAsync); - _strategy = FileStreamHelpers.ChooseStrategy(this, handle, access, bufferSize, isAsync); + _strategy = FileStreamHelpers.ChooseStrategy(this, handle, access, DefaultShare, bufferSize, isAsync); } public FileStream(string path, FileMode mode) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs index 1209f5eedd1c2..b7f24351a0bda 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs @@ -13,8 +13,8 @@ internal sealed partial class AsyncWindowsFileStreamStrategy : WindowsFileStream private PreAllocatedOverlapped? _preallocatedOverlapped; // optimization for async ops to avoid per-op allocations private FileStreamCompletionSource? _currentOverlappedOwner; // async op currently using the preallocated overlapped - internal AsyncWindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access) - : base(handle, access) + internal AsyncWindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access, FileShare share) + : base(handle, access, share) { } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs index 8d56567233ca6..52b5568b1f05f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs @@ -19,7 +19,7 @@ internal static partial class FileStreamHelpers private const int ERROR_HANDLE_EOF = 38; private const int ERROR_IO_PENDING = 997; - private static FileStreamStrategy ChooseStrategyCore(SafeFileHandle handle, FileAccess access, int bufferSize, bool isAsync) + private static FileStreamStrategy ChooseStrategyCore(SafeFileHandle handle, FileAccess access, FileShare share, int bufferSize, bool isAsync) { if (UseLegacyStrategy) { @@ -27,8 +27,8 @@ private static FileStreamStrategy ChooseStrategyCore(SafeFileHandle handle, File } WindowsFileStreamStrategy strategy = isAsync - ? new AsyncWindowsFileStreamStrategy(handle, access) - : new SyncWindowsFileStreamStrategy(handle, access); + ? new AsyncWindowsFileStreamStrategy(handle, access, share) + : new SyncWindowsFileStreamStrategy(handle, access, share); return EnableBufferingIfNeeded(strategy, bufferSize); } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.cs index 5eb9eabaeeb97..62e344db6e4f0 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.cs @@ -23,8 +23,8 @@ private static bool GetLegacyFileStreamSetting() : bool.IsTrueStringIgnoreCase(envVar) || envVar.Equals("1"); } - internal static FileStreamStrategy ChooseStrategy(FileStream fileStream, SafeFileHandle handle, FileAccess access, int bufferSize, bool isAsync) - => WrapIfDerivedType(fileStream, ChooseStrategyCore(handle, access, bufferSize, isAsync)); + internal static FileStreamStrategy ChooseStrategy(FileStream fileStream, SafeFileHandle handle, FileAccess access, FileShare share, int bufferSize, bool isAsync) + => WrapIfDerivedType(fileStream, ChooseStrategyCore(handle, access, share, bufferSize, isAsync)); internal static FileStreamStrategy ChooseStrategy(FileStream fileStream, string path, FileMode mode, FileAccess access, FileShare share, int bufferSize, FileOptions options) => WrapIfDerivedType(fileStream, ChooseStrategyCore(path, mode, access, share, bufferSize, options)); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/SyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/SyncWindowsFileStreamStrategy.cs index 4545e1dff0b15..3639b4b5fb4da 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/SyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/SyncWindowsFileStreamStrategy.cs @@ -11,7 +11,7 @@ namespace System.IO.Strategies { internal sealed class SyncWindowsFileStreamStrategy : WindowsFileStreamStrategy { - internal SyncWindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access) : base(handle, access) + internal SyncWindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access, FileShare share) : base(handle, access, share) { } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs index 9aa9d9c0bbf3e..614ef55b98830 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs @@ -27,16 +27,16 @@ internal abstract class WindowsFileStreamStrategy : FileStreamStrategy protected long _filePosition; private long _appendStart; // When appending, prevent overwriting file. - private long? _length; + private long _length = -1; // When the FileStream blocks the handle (_share <= FileShare.Read) keep file length in-memory, negative means that hasn't been fetched. - internal WindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access) + internal WindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access, FileShare share) { InitFromHandle(handle, access, out _canSeek, out _isPipe); // Note: Cleaner to set the following fields in ValidateAndInitFromHandle, // but we can't as they're readonly. _access = access; - _share = FileStream.DefaultShare; + _share = share; // As the handle was passed in, we must set the handle field at the very end to // avoid the finalizer closing the handle when we throw errors. @@ -77,15 +77,15 @@ internal WindowsFileStreamStrategy(string path, FileMode mode, FileAccess access public unsafe sealed override long Length => _share > FileShare.Read ? FileStreamHelpers.GetFileLength(_fileHandle, _path) : - _length ??= FileStreamHelpers.GetFileLength(_fileHandle, _path); + _length < 0 ? _length = FileStreamHelpers.GetFileLength(_fileHandle, _path) : _length; protected void UpdateLengthOnChangePosition() { // Do not update the cached length if the file can be written somewhere else - // or if the length has not been queried. - if (_share > FileShare.Read || _length is null) + // or if the length hasn't been fetched. + if (_share > FileShare.Read || _length < 0) { - Debug.Assert(_length is null); + Debug.Assert(_length < 0); return; } From 3bb910ab4a1be50304e22d8da0d4f58070072227 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 22 Mar 2021 08:26:53 +0100 Subject: [PATCH 07/11] fix the Unix build? --- .../src/System/IO/Strategies/FileStreamHelpers.Unix.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Unix.cs index 4d3639185feb4..9af81a5136d04 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Unix.cs @@ -14,7 +14,7 @@ namespace System.IO.Strategies internal static partial class FileStreamHelpers { // in the future we are most probably going to introduce more strategies (io_uring etc) - private static FileStreamStrategy ChooseStrategyCore(SafeFileHandle handle, FileAccess access, int bufferSize, bool isAsync) + private static FileStreamStrategy ChooseStrategyCore(SafeFileHandle handle, FileAccess access, FileShare share, int bufferSize, bool isAsync) => new LegacyFileStreamStrategy(handle, access, bufferSize, isAsync); private static FileStreamStrategy ChooseStrategyCore(string path, FileMode mode, FileAccess access, FileShare share, int bufferSize, FileOptions options) From 1d388087b969a896e06fb1c4346a9f0f09d7b557 Mon Sep 17 00:00:00 2001 From: David Cantu Date: Mon, 22 Mar 2021 10:26:00 -0700 Subject: [PATCH 08/11] Address suggestions --- .../IO/Strategies/FileStreamHelpers.Windows.cs | 9 --------- .../IO/Strategies/WindowsFileStreamStrategy.cs | 14 ++++++++++---- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs index 52b5568b1f05f..661e84b10f670 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs @@ -389,15 +389,6 @@ internal static unsafe int WriteFileNative(SafeFileHandle handle, ReadOnlySpan !_fileHandle.IsClosed && (_access & FileAccess.Write) != 0; + // When the handle blocks the file we can keep file length in memory + // and avoid subsequent native calls which are expensive. public unsafe sealed override long Length => _share > FileShare.Read ? FileStreamHelpers.GetFileLength(_fileHandle, _path) : _length < 0 ? _length = FileStreamHelpers.GetFileLength(_fileHandle, _path) : _length; protected void UpdateLengthOnChangePosition() { - // Do not update the cached length if the file can be written somewhere else + // Do not update the cached length if the file is not blocked // or if the length hasn't been fetched. if (_share > FileShare.Read || _length < 0) { @@ -112,8 +114,12 @@ internal sealed override SafeFileHandle SafeFileHandle { get { - // Update the file offset in the handle before exposing it. - FileStreamHelpers.Seek(_fileHandle, _path, _filePosition, SeekOrigin.Begin); + if (CanSeek) + { + // Update the file offset before exposing it since it's possible that + // in memory position is out-of-sync with the actual file position. + FileStreamHelpers.Seek(_fileHandle, _path, _filePosition, SeekOrigin.Begin); + } return _fileHandle; } } From ff4e8624ed327f1ed1da7176f3491ecb650f8aec Mon Sep 17 00:00:00 2001 From: David Cantu Date: Mon, 22 Mar 2021 10:29:40 -0700 Subject: [PATCH 09/11] Remove more ERROR_HANDLE_EOF validations from Write* methods --- .../IO/Strategies/AsyncWindowsFileStreamStrategy.cs | 10 +--------- .../IO/Strategies/LegacyFileStreamStrategy.Windows.cs | 10 +--------- 2 files changed, 2 insertions(+), 18 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs index b7f24351a0bda..fdaf2dfaa508e 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs @@ -306,15 +306,7 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, Cancella } completionSource.ReleaseNativeResource(); - - if (errorCode == ERROR_HANDLE_EOF) - { - ThrowHelper.ThrowEndOfFileException(); - } - else - { - throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); - } + throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); } else if (cancellationToken.CanBeCanceled) // ERROR_IO_PENDING { diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/LegacyFileStreamStrategy.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/LegacyFileStreamStrategy.Windows.cs index e562f0c2e2151..6b89ef093577c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/LegacyFileStreamStrategy.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/LegacyFileStreamStrategy.Windows.cs @@ -1037,15 +1037,7 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, Cancella } completionSource.ReleaseNativeResource(); - - if (errorCode == ERROR_HANDLE_EOF) - { - ThrowHelper.ThrowEndOfFileException(); - } - else - { - throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); - } + throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); } else if (cancellationToken.CanBeCanceled) // ERROR_IO_PENDING { From 8bc05eca642a95de54815146a76cc58641bbdc1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Cant=C3=BA?= Date: Mon, 22 Mar 2021 15:06:46 -0700 Subject: [PATCH 10/11] Apply suggestions from code review Co-authored-by: Adam Sitnik --- .../src/System/IO/Strategies/WindowsFileStreamStrategy.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs index b581dbd2bad82..c178c5e47daa6 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs @@ -27,7 +27,7 @@ internal abstract class WindowsFileStreamStrategy : FileStreamStrategy protected long _filePosition; private long _appendStart; // When appending, prevent overwriting file. - private long _length = -1; // When the handle blocks the file (_share <= FileShare.Read) keep file length in-memory, negative means that hasn't been fetched. + private long _length = -1; // When the file is locked for writes (_share <= FileShare.Read) cache file length in-memory, negative means that hasn't been fetched. internal WindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access, FileShare share) { @@ -75,7 +75,7 @@ internal WindowsFileStreamStrategy(string path, FileMode mode, FileAccess access public sealed override bool CanWrite => !_fileHandle.IsClosed && (_access & FileAccess.Write) != 0; - // When the handle blocks the file we can keep file length in memory + // When the file is locked for writes we can cache file length in memory // and avoid subsequent native calls which are expensive. public unsafe sealed override long Length => _share > FileShare.Read ? FileStreamHelpers.GetFileLength(_fileHandle, _path) : @@ -83,7 +83,7 @@ internal WindowsFileStreamStrategy(string path, FileMode mode, FileAccess access protected void UpdateLengthOnChangePosition() { - // Do not update the cached length if the file is not blocked + // Do not update the cached length if the file is not locked // or if the length hasn't been fetched. if (_share > FileShare.Read || _length < 0) { From 31a0ed23543afbd0f9b625bc5edc6fbd2174dc90 Mon Sep 17 00:00:00 2001 From: David Cantu Date: Mon, 22 Mar 2021 15:07:36 -0700 Subject: [PATCH 11/11] Revert "Remove more ERROR_HANDLE_EOF validations from Write* methods" This reverts commit ff4e8624ed327f1ed1da7176f3491ecb650f8aec. --- .../IO/Strategies/AsyncWindowsFileStreamStrategy.cs | 10 +++++++++- .../IO/Strategies/LegacyFileStreamStrategy.Windows.cs | 10 +++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs index fdaf2dfaa508e..b7f24351a0bda 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs @@ -306,7 +306,15 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, Cancella } completionSource.ReleaseNativeResource(); - throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); + + if (errorCode == ERROR_HANDLE_EOF) + { + ThrowHelper.ThrowEndOfFileException(); + } + else + { + throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); + } } else if (cancellationToken.CanBeCanceled) // ERROR_IO_PENDING { diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/LegacyFileStreamStrategy.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/LegacyFileStreamStrategy.Windows.cs index 6b89ef093577c..e562f0c2e2151 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/LegacyFileStreamStrategy.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/LegacyFileStreamStrategy.Windows.cs @@ -1037,7 +1037,15 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, Cancella } completionSource.ReleaseNativeResource(); - throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); + + if (errorCode == ERROR_HANDLE_EOF) + { + ThrowHelper.ThrowEndOfFileException(); + } + else + { + throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); + } } else if (cancellationToken.CanBeCanceled) // ERROR_IO_PENDING {