Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend FileStreamOptions with BufferSize, allow to specify 0 to disable the buffering #52928

Merged
merged 3 commits into from
May 19, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ public async Task NoDataIsLostWhenWritingToFile(ReadWriteMode mode)
filePath = stream.Name;

// the following buffer fits into internal FileStream buffer
byte[] small = Enumerable.Repeat(byte.MinValue, BufferSize - 1).ToArray();
byte[] small = Enumerable.Repeat(byte.MinValue, Math.Max(1, BufferSize - 1)).ToArray();
// the following buffer does not fit into internal FileStream buffer
byte[] big = Enumerable.Repeat(byte.MaxValue, BufferSize + 1).ToArray();
// in this test we are selecting a random buffer and write it to file
Expand Down Expand Up @@ -175,7 +175,7 @@ public async Task WriteByteFlushesTheBufferWhenItBecomesFull()
stream.WriteByte(0);
writtenBytes.Add(0);

byte[] bytes = new byte[BufferSize - 1];
byte[] bytes = new byte[Math.Max(0, BufferSize - 1)];
stream.Write(bytes.AsSpan());
writtenBytes.AddRange(bytes);

Expand All @@ -191,7 +191,12 @@ public async Task WriteByteFlushesTheBufferWhenItBecomesFull()
public class UnbufferedSyncFileStreamStandaloneConformanceTests : FileStreamStandaloneConformanceTests
{
protected override FileOptions Options => FileOptions.None;

#if RELEASE // since buffering can be now disabled by setting the buffer size to 0 or 1, let's test 0 in one config and 1 in the other
protected override int BufferSize => 0;
#else
protected override int BufferSize => 1;
#endif
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved
}

public class BufferedSyncFileStreamStandaloneConformanceTests : FileStreamStandaloneConformanceTests
Expand All @@ -205,7 +210,12 @@ public class BufferedSyncFileStreamStandaloneConformanceTests : FileStreamStanda
public class UnbufferedAsyncFileStreamStandaloneConformanceTests : FileStreamStandaloneConformanceTests
{
protected override FileOptions Options => FileOptions.Asynchronous;

#if RELEASE
protected override int BufferSize => 0;
#else
protected override int BufferSize => 1;
#endif
}

[ActiveIssue("https://github.com/dotnet/runtime/issues/34583", TestPlatforms.Windows, TargetFrameworkMonikers.Netcoreapp, TestRuntimes.Mono)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,18 @@ protected override FileStream CreateFileStream(string path, FileMode mode, FileA
PreallocationSize = PreallocationSize
});

protected override FileStream CreateFileStream(string path, FileMode mode, FileAccess access, FileShare share, int bufferSize, FileOptions options)
=> new FileStream(path,
new FileStreamOptions
{
Mode = mode,
Access = access,
Share = share,
BufferSize = bufferSize,
Options = options,
PreallocationSize = PreallocationSize
});
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved

protected FileStreamOptions GetOptions(FileMode mode, FileAccess access, FileShare share, FileOptions options, long preAllocationSize)
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved
=> new FileStreamOptions
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,11 @@ public void NegativeBufferSizeThrows()
}

[Fact]
public void ZeroBufferSizeThrows()
public void ZeroBufferSizeDoesNotThrow()
{
// Unfortunate pre-existing behavior of FileStream, we should look into enabling this sometime.
AssertExtensions.Throws<ArgumentOutOfRangeException>("bufferSize", () => CreateFileStream(GetTestFilePath(), FileMode.Create, FileAccess.ReadWrite, FileShare.Read, 0));
using (CreateFileStream(GetTestFilePath(), FileMode.Create, FileAccess.ReadWrite, FileShare.Read, 0))
{
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be better to just change ValidBufferSize below to be a theory, with [InlineData(0)], [InlineData(64 * 1024)], and whatever other sizes we want to validate.


[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,9 @@ public FileStream(string path, FileMode mode, FileAccess access, FileShare share
/// -or-
/// <paramref name="path" /> refers to a non-file device, such as <c>CON:</c>, <c>COM1:</c>, <c>LPT1:</c>, etc. in an NTFS environment.</exception>
/// <exception cref="T:System.NotSupportedException"><paramref name="path" /> refers to a non-file device, such as <c>CON:</c>, <c>COM1:</c>, <c>LPT1:</c>, etc. in a non-NTFS environment.</exception>
/// <exception cref="T:System.ArgumentOutOfRangeException"><see cref="System.IO.FileStreamOptions.PreallocationSize" /> is negative.
/// <exception cref="T:System.ArgumentOutOfRangeException"><see cref="System.IO.FileStreamOptions.BufferSize" /> is negative.
/// -or-
/// <see cref="System.IO.FileStreamOptions.PreallocationSize" /> is negative.
/// -or-
/// <see cref="System.IO.FileStreamOptions.Mode" />, <see cref="System.IO.FileStreamOptions.Access" />, or <see cref="System.IO.FileStreamOptions.Share" /> contain an invalid value.</exception>
/// <exception cref="T:System.IO.FileNotFoundException">The file cannot be found, such as when <see cref="System.IO.FileStreamOptions.Mode" /> is <see langword="FileMode.Truncate" /> or <see langword="FileMode.Open" />, and the file specified by <paramref name="path" /> does not exist. The file must already exist in these modes.</exception>
Expand All @@ -167,7 +169,7 @@ public FileStream(string path, FileMode mode, FileAccess access, FileShare share
/// <see cref="F:System.IO.FileOptions.Encrypted" /> is specified for <see cref="System.IO.FileStreamOptions.Options" /> , but file encryption is not supported on the current platform.</exception>
/// <exception cref="T:System.IO.PathTooLongException">The specified path, file name, or both exceed the system-defined maximum length. </exception>
public FileStream(string path, FileStreamOptions options)
: this(path, options.Mode, options.Access, options.Share, DefaultBufferSize, options.Options, options.PreallocationSize)
: this(path, options.Mode, options.Access, options.Share, options.BufferSize, options.Options, options.PreallocationSize)
{
}

Expand Down Expand Up @@ -209,7 +211,7 @@ private FileStream(string path, FileMode mode, FileAccess access, FileShare shar
{
throw new ArgumentOutOfRangeException(nameof(options), SR.ArgumentOutOfRange_Enum);
}
else if (bufferSize <= 0)
else if (bufferSize < 0)
{
throw new ArgumentOutOfRangeException(nameof(bufferSize), SR.ArgumentOutOfRange_NeedPosNum);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,10 @@ public sealed class FileStreamOptions
/// In other cases (including the default 0 value), it's ignored.
/// </summary>
public long PreallocationSize { get; set; }
/// <summary>
/// The size of the buffer used by <see cref="FileStream" /> for buffering. The default buffer size is 4096.
/// 0 or 1 means that buffering should be disabled. Negative values are not allowed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Negative values are not allowed.

We don't want to do such validation in the setter?

/// </summary>
public int BufferSize { get; set; } = 4096;
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ 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, FileShare share, int bufferSize, bool isAsync)
=> new Net5CompatFileStreamStrategy(handle, access, bufferSize, isAsync);
=> new Net5CompatFileStreamStrategy(handle, access, bufferSize == 0 ? 1 : bufferSize, isAsync);

private static FileStreamStrategy ChooseStrategyCore(string path, FileMode mode, FileAccess access, FileShare share, int bufferSize, FileOptions options, long preallocationSize)
=> new Net5CompatFileStreamStrategy(path, mode, access, share, bufferSize, options, preallocationSize);
=> new Net5CompatFileStreamStrategy(path, mode, access, share, bufferSize == 0 ? 1 : bufferSize, options, preallocationSize);

internal static SafeFileHandle OpenHandle(string path, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ private static FileStreamStrategy ChooseStrategyCore(SafeFileHandle handle, File
{
if (UseNet5CompatStrategy)
{
return new Net5CompatFileStreamStrategy(handle, access, bufferSize, isAsync);
// The .NET 5 Compat strategy does not support bufferSize == 0.
// To minimize the risk of introducing bugs to it, we just pass 1 to disable the buffering.
return new Net5CompatFileStreamStrategy(handle, access, bufferSize == 0 ? 1 : bufferSize, isAsync);
}

WindowsFileStreamStrategy strategy = isAsync
Expand All @@ -46,7 +48,7 @@ private static FileStreamStrategy ChooseStrategyCore(string path, FileMode mode,
{
if (UseNet5CompatStrategy)
{
return new Net5CompatFileStreamStrategy(path, mode, access, share, bufferSize, options, preallocationSize);
return new Net5CompatFileStreamStrategy(path, mode, access, share, bufferSize == 0 ? 1 : bufferSize, options, preallocationSize);
}

WindowsFileStreamStrategy strategy = (options & FileOptions.Asynchronous) != 0
Expand All @@ -57,7 +59,7 @@ private static FileStreamStrategy ChooseStrategyCore(string path, FileMode mode,
}

internal static FileStreamStrategy EnableBufferingIfNeeded(WindowsFileStreamStrategy strategy, int bufferSize)
=> bufferSize == 1 ? strategy : new BufferedFileStreamStrategy(strategy, bufferSize);
=> bufferSize <= 1 ? strategy : new BufferedFileStreamStrategy(strategy, bufferSize);
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved

internal static SafeFileHandle OpenHandle(string path, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize)
=> CreateFileOpenHandle(path, mode, access, share, options, preallocationSize);
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Runtime/ref/System.Runtime.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7307,6 +7307,7 @@ public sealed class FileStreamOptions
public System.IO.FileShare Share { get; set; }
public System.IO.FileOptions Options { get; set; }
public long PreallocationSize { get; set; }
public int BufferSize { get; set; }
}
public partial class FileStream : System.IO.Stream
{
Expand Down