From 2a59639f8d43f0d790a2a7132a79492284d6e323 Mon Sep 17 00:00:00 2001 From: pedrobsaila Date: Tue, 29 Nov 2022 19:54:35 +0100 Subject: [PATCH 1/6] Unify BufferedFileStreamStrategy seeking --- .../System/IO/Strategies/BufferedFileStreamStrategy.cs | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs index 9e9700339014d8..24c6317eaec1b7 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs @@ -65,15 +65,7 @@ public override long Position } set { - if (_writePos > 0) - { - FlushWrite(); - } - - _readPos = 0; - _readLen = 0; - - _strategy.Position = value; + Seek(value, SeekOrigin.Begin); } } From 356010eb8b9477e70ae8ecff2cb29c4e1a15a5ea Mon Sep 17 00:00:00 2001 From: pedrobsaila Date: Sat, 3 Dec 2022 13:51:45 +0100 Subject: [PATCH 2/6] make Position setter clearer --- .../src/System/IO/FileStream.cs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) 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 83b406d4eec90d..ea705168048ff2 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs @@ -488,7 +488,16 @@ public override long Position ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.value, ExceptionResource.ArgumentOutOfRange_NeedNonNegNum); } - _strategy.Seek(value, SeekOrigin.Begin); + if (_strategy.IsClosed) + { + ThrowHelper.ThrowObjectDisposedException_FileClosed(); + } + else if (!CanSeek) + { + ThrowHelper.ThrowNotSupportedException_UnseekableStream(); + } + + _strategy.Position = value; } } From 0acbfbd8f98ebaa64404379fba2b44124b08bdfa Mon Sep 17 00:00:00 2001 From: pedrobsaila Date: Sat, 3 Dec 2022 13:53:01 +0100 Subject: [PATCH 3/6] add else --- .../System.Private.CoreLib/src/System/IO/FileStream.cs | 3 +-- 1 file changed, 1 insertion(+), 2 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 ea705168048ff2..5a3bb83bf424b9 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs @@ -487,8 +487,7 @@ public override long Position { ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.value, ExceptionResource.ArgumentOutOfRange_NeedNonNegNum); } - - if (_strategy.IsClosed) + else if (_strategy.IsClosed) { ThrowHelper.ThrowObjectDisposedException_FileClosed(); } From a75268678af3fc42faab027e6140c7151dc037af Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 23 Dec 2022 15:28:23 +0100 Subject: [PATCH 4/6] add missing test coverage --- .../System.IO.FileSystem/tests/FileStream/Seek.cs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/Seek.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/Seek.cs index 963a1fddff57d4..d934fa2b7730de 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/Seek.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/Seek.cs @@ -7,8 +7,10 @@ namespace System.IO.Tests { public class FileStream_Seek : FileSystemTest { - [Fact] - public void SeekAppendModifyThrows() + [Theory] + [InlineData(0)] + [InlineData(10)] + public void SeekAppendModifyThrows(int bufferSize) { string fileName = GetTestFilePath(); using (FileStream fs = new FileStream(fileName, FileMode.Create)) @@ -16,7 +18,7 @@ public void SeekAppendModifyThrows() fs.Write(TestBuffer, 0, TestBuffer.Length); } - using (FileStream fs = new FileStream(fileName, FileMode.Append)) + using (FileStream fs = new FileStream(fileName, FileMode.Append, FileAccess.Write, FileShare.Read, bufferSize)) { long length = fs.Length; Assert.Throws(() => fs.Seek(length - 1, SeekOrigin.Begin)); @@ -33,6 +35,9 @@ public void SeekAppendModifyThrows() Assert.Throws(() => fs.Seek(-length, SeekOrigin.End)); Assert.Equal(length, fs.Position); + Assert.Throws(() => fs.Position = length - 1); + Assert.Equal(length, fs.Position); + fs.Write(TestBuffer); Assert.Equal(length, fs.Seek(length, SeekOrigin.Begin)); } From 5f7dc6c76acdbc3f10322a7c23afea5202a3d4a1 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 23 Dec 2022 16:02:36 +0100 Subject: [PATCH 5/6] move the validation logic to FileStream so each strategy does not need to duplicate it --- .../src/System/IO/FileStream.cs | 28 +++++++++++++++---- .../Strategies/BufferedFileStreamStrategy.cs | 3 -- .../IO/Strategies/OSFileStreamStrategy.cs | 5 ---- 3 files changed, 23 insertions(+), 13 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 5a3bb83bf424b9..9fa6984f59e4b9 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs @@ -487,12 +487,13 @@ public override long Position { ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.value, ExceptionResource.ArgumentOutOfRange_NeedNonNegNum); } - else if (_strategy.IsClosed) - { - ThrowHelper.ThrowObjectDisposedException_FileClosed(); - } else if (!CanSeek) { + if(_strategy.IsClosed) + { + ThrowHelper.ThrowObjectDisposedException_FileClosed(); + } + ThrowHelper.ThrowNotSupportedException_UnseekableStream(); } @@ -583,7 +584,24 @@ public override void EndWrite(IAsyncResult asyncResult) public override bool CanSeek => _strategy.CanSeek; - public override long Seek(long offset, SeekOrigin origin) => _strategy.Seek(offset, origin); + public override long Seek(long offset, SeekOrigin origin) + { + if (origin < SeekOrigin.Begin || origin > SeekOrigin.End) + { + throw new ArgumentException(SR.Argument_InvalidSeekOrigin, nameof(origin)); + } + else if (!CanSeek) + { + if (_strategy.IsClosed) + { + ThrowHelper.ThrowObjectDisposedException_FileClosed(); + } + + ThrowHelper.ThrowNotSupportedException_UnseekableStream(); + } + + return _strategy.Seek(offset, origin); + } internal Task BaseFlushAsync(CancellationToken cancellationToken) => base.FlushAsync(cancellationToken); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs index 24c6317eaec1b7..c2227e72469b24 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs @@ -896,9 +896,6 @@ public override void CopyTo(Stream destination, int bufferSize) public override long Seek(long offset, SeekOrigin origin) { - EnsureNotClosed(); - EnsureCanSeek(); - // If we have bytes in the write buffer, flush them out, seek and be done. if (_writePos > 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 43f7152167cd3d..89d1ccf7f6ab0f 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 @@ -144,11 +144,6 @@ internal sealed override void Flush(bool flushToDisk) public sealed override long Seek(long offset, SeekOrigin origin) { - if (origin < SeekOrigin.Begin || origin > SeekOrigin.End) - throw new ArgumentException(SR.Argument_InvalidSeekOrigin, nameof(origin)); - if (_fileHandle.IsClosed) ThrowHelper.ThrowObjectDisposedException_FileClosed(); - if (!CanSeek) ThrowHelper.ThrowNotSupportedException_UnseekableStream(); - long oldPos = _filePosition; long pos = origin switch { From cc0e737ed842d4213c396d77bc58095889fc8872 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 23 Dec 2022 16:07:18 +0100 Subject: [PATCH 6/6] OSFileStreamStrategy.Position should delegate to Seek too --- .../src/System/IO/Strategies/OSFileStreamStrategy.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 89d1ccf7f6ab0f..b9f6c9eeccbea5 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 @@ -85,7 +85,7 @@ internal void OnIncompleteOperation(int expectedBytesTransferred, int actualByte public sealed override long Position { get => _filePosition; - set => _filePosition = value; + set => Seek(value, SeekOrigin.Begin); } internal sealed override string Name => _fileHandle.Path ?? SR.IO_UnknownFileName;