diff --git a/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs b/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs index d44f9bcfdd6614..f8d0a052995a77 100644 --- a/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs +++ b/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs @@ -290,6 +290,8 @@ private static Version GetICUVersion() public static bool IsNet5CompatFileStreamEnabled => _net5CompatFileStream.Value; + public static bool IsNet5CompatFileStreamDisabled => !_net5CompatFileStream.Value; + private static readonly Lazy s_fileLockingDisabled = new Lazy(() => GetStaticNonPublicBooleanPropertyValue("Microsoft.Win32.SafeHandles.SafeFileHandle", "DisableFileLocking")); public static bool IsFileLockingEnabled => IsWindows || !s_fileLockingDisabled.Value; diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/ReadAsync.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/ReadAsync.cs index 9985119b35d0d0..ecb89d35ed315d 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/ReadAsync.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/ReadAsync.cs @@ -132,6 +132,36 @@ public async Task IncompleteReadCantSetPositionBeyondEndOfFile(FileShare fileSha Assert.Equal(fileSize, fs.Position); } } + + [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported), nameof(PlatformDetection.IsNet5CompatFileStreamDisabled))] + [InlineData(FileShare.None, FileOptions.Asynchronous)] // FileShare.None: exclusive access + [InlineData(FileShare.ReadWrite, FileOptions.Asynchronous)] // FileShare.ReadWrite: others can write to the file, the length can't be cached + [InlineData(FileShare.None, FileOptions.None)] + [InlineData(FileShare.ReadWrite, FileOptions.None)] + public async Task ConcurrentReadsKeepCorrectOrder(FileShare fileShare, FileOptions options) + { + const int fileSize = 10_000; + string filePath = GetTestFilePath(); + byte[] content = RandomNumberGenerator.GetBytes(fileSize); + File.WriteAllBytes(filePath, content); + + byte[] buffer = new byte[fileSize]; + + using (FileStream fs = new FileStream(filePath, FileMode.Open, FileAccess.Read, fileShare, bufferSize: 0, options)) + { + Task[] reads = Enumerable.Range(0, 10).Select(index => ReadAsync(fs, buffer, index * 1000, 1000)).ToArray(); + + // the reads were not awaited, it's an anti-pattern and Position can be (0, fileSize) now: + Assert.InRange(fs.Position, 0, fileSize); + + await Task.WhenAll(reads); + + Assert.All(reads, read => Assert.Equal(1000, read.Result)); + AssertExtensions.SequenceEqual(content, buffer); + Assert.Equal(fileSize, fs.Position); + Assert.Equal(fileSize, fs.Length); + } + } } [ActiveIssue("https://github.com/dotnet/runtime/issues/34582", TestPlatforms.Windows, TargetFrameworkMonikers.Netcoreapp, TestRuntimes.Mono)] diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/WriteAsync.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/WriteAsync.cs index 06a9422589ee84..260f28780c6575 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/WriteAsync.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/WriteAsync.cs @@ -1,6 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Linq; +using System.Security.Cryptography; using System.Collections.Generic; using System.Threading; using System.Threading.Tasks; @@ -161,6 +163,34 @@ public async Task WriteAsyncInternalBufferOverflow() } } + [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported), nameof(PlatformDetection.IsNet5CompatFileStreamDisabled))] + [InlineData(FileShare.None, FileOptions.Asynchronous)] // FileShare.None: exclusive access + [InlineData(FileShare.ReadWrite, FileOptions.Asynchronous)] // FileShare.ReadWrite: others can write to the file, the length can't be cached + [InlineData(FileShare.None, FileOptions.None)] + [InlineData(FileShare.ReadWrite, FileOptions.None)] + public async Task ConcurrentWritesKeepCorrectOrder(FileShare fileShare, FileOptions options) + { + const int fileSize = 10_000; + string filePath = GetTestFilePath(); + byte[] content = RandomNumberGenerator.GetBytes(fileSize); + + using (FileStream fs = new FileStream(filePath, FileMode.CreateNew, FileAccess.Write, fileShare, bufferSize: 0, options)) + { + Task[] writes = Enumerable.Range(0, 10).Select(index => WriteAsync(fs, content, index * 1000, 1000)).ToArray(); + + // the writes were not awaited, it's an anti-pattern and Position can be (0, fileSize) now: + Assert.InRange(fs.Position, 0, fileSize); + + await Task.WhenAll(writes); + + Assert.Equal(fileSize, fs.Position); + Assert.Equal(fileSize, fs.Length); + Assert.All(writes, write => Assert.True(write.IsCompletedSuccessfully)); + } + + AssertExtensions.SequenceEqual(content, await File.ReadAllBytesAsync(filePath)); + } + public static IEnumerable MemberData_FileStreamAsyncWriting() { foreach (bool useAsync in new[] { true, false }) diff --git a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.OverlappedValueTaskSource.Windows.cs b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.OverlappedValueTaskSource.Windows.cs index 1ba3516c42a9f1..ed59f84aa8bb0a 100644 --- a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.OverlappedValueTaskSource.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.OverlappedValueTaskSource.Windows.cs @@ -200,10 +200,7 @@ internal void Complete(uint errorCode, uint numBytes) OSFileStreamStrategy? strategy = _strategy; ReleaseResources(); - if (strategy is not null && _bufferSize != numBytes) // true only for incomplete operations - { - strategy.OnIncompleteOperation(_bufferSize, (int)numBytes); - } + strategy?.OnFinishedAsyncOperation(_bufferSize, (int)numBytes); switch (errorCode) { diff --git a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.ThreadPoolValueTaskSource.cs b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.ThreadPoolValueTaskSource.cs index 9fff83001662bd..41d5724a97c6ce 100644 --- a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.ThreadPoolValueTaskSource.cs +++ b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.ThreadPoolValueTaskSource.cs @@ -100,6 +100,7 @@ private void ExecuteInternal() break; case Operation.Write: RandomAccess.WriteAtOffset(_fileHandle, _singleSegment.Span, _fileOffset); + result = _singleSegment.Length; break; case Operation.ReadScatter: Debug.Assert(_readScatterBuffers != null); @@ -118,18 +119,7 @@ private void ExecuteInternal() } finally { - if (_strategy is not null) - { - // WriteAtOffset returns void, so we need to fix position only in case of an exception - if (exception is not null) - { - _strategy.OnIncompleteOperation(_singleSegment.Length, 0); - } - else if (_operation == Operation.Read && result != _singleSegment.Length) - { - _strategy.OnIncompleteOperation(_singleSegment.Length, (int)result); - } - } + _strategy?.OnFinishedAsyncOperation(_singleSegment.Length, (int)result); _operation = Operation.None; _context = null; 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 d367abb8fb9736..7fcaa305460662 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 @@ -293,7 +293,7 @@ private static unsafe (SafeFileHandle.OverlappedValueTaskSource? vts, int errorC { if (errorCode != Interop.Errors.ERROR_IO_PENDING && errorCode != Interop.Errors.ERROR_SUCCESS) { - strategy?.OnIncompleteOperation(buffer.Length, 0); + strategy?.OnFinishedAsyncOperation(buffer.Length, 0); } } @@ -368,7 +368,7 @@ private static unsafe (SafeFileHandle.OverlappedValueTaskSource? vts, int errorC { if (errorCode != Interop.Errors.ERROR_IO_PENDING && errorCode != Interop.Errors.ERROR_SUCCESS) { - strategy?.OnIncompleteOperation(buffer.Length, 0); + strategy?.OnFinishedAsyncOperation(buffer.Length, 0); } } 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..0728c2d626ac3f 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 @@ -15,7 +14,8 @@ internal abstract class OSFileStreamStrategy : FileStreamStrategy protected readonly SafeFileHandle _fileHandle; // only ever null if ctor throws private readonly FileAccess _access; // What file was opened for. - protected long _filePosition; + protected long _filePosition; // updated after every operation finishes + private long _nextAsyncOperationOffset; // updated before async operation starts and after it finishes 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. @@ -101,8 +101,15 @@ public unsafe sealed override long Length // 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); + internal void OnFinishedAsyncOperation(int expectedBytesTransferred, int actualBytesTransferred) + { + if (actualBytesTransferred > 0) + { + _filePosition += actualBytesTransferred; + } + + Interlocked.Add(ref _nextAsyncOperationOffset, -expectedBytesTransferred); + } private bool LengthCachingSupported => OperatingSystem.IsWindows() && _lengthCanBeCached; @@ -294,7 +301,7 @@ public sealed override Task WriteAsync(byte[] buffer, int offset, int count, Can public sealed override ValueTask WriteAsync(ReadOnlyMemory source, CancellationToken cancellationToken) { - long writeOffset = CanSeek ? Interlocked.Add(ref _filePosition, source.Length) - source.Length : -1; + long writeOffset = CanSeek ? _filePosition + Interlocked.Add(ref _nextAsyncOperationOffset, source.Length) - source.Length : -1; return RandomAccess.WriteAtOffsetAsync(_fileHandle, source, writeOffset, cancellationToken, this); } @@ -324,7 +331,7 @@ public sealed override ValueTask ReadAsync(Memory destination, Cancel // This implementation updates the file position before the operation starts and updates it after incomplete read. // This is done to keep backward compatibility for concurrent reads. // It uses Interlocked as there can be multiple concurrent incomplete reads updating position at the same time. - long readOffset = Interlocked.Add(ref _filePosition, destination.Length) - destination.Length; + long readOffset = _filePosition + Interlocked.Add(ref _nextAsyncOperationOffset, destination.Length) - destination.Length; return RandomAccess.ReadAtOffsetAsync(_fileHandle, destination, readOffset, cancellationToken, this); } }