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

Conversation

adamsitnik
Copy link
Member

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented May 18, 2021

Tagging subscribers to this area: @carlossanlop
See info in area-owners.md if you want to be subscribed.

Issue Details

#15088 (comment)

Author: adamsitnik
Assignees: -
Labels:

area-System.IO

Milestone: 6.0.0

Copy link
Member

@Jozkee Jozkee left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM, thanks.

System.IO - FileStream automation moved this from In progress to Reviewer approved May 18, 2021
Co-authored-by: David Cantú <dacantu@microsoft.com>
@adamsitnik adamsitnik merged commit 82289b2 into dotnet:main May 19, 2021
System.IO - FileStream automation moved this from Reviewer approved to Done May 19, 2021
@adamsitnik adamsitnik deleted the bufferSize branch May 19, 2021 07:58
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.

@@ -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?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants