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

Allow for passing zero to disable buffering for methods that accept bufferSize and use FileStream #53774

Closed
wants to merge 2 commits into from

Conversation

Marusyk
Copy link
Member

@Marusyk Marusyk commented Jun 5, 2021

@adamsitnik could you please check if I on the right way?

Issue #53497

@ghost
Copy link

ghost commented Jun 5, 2021

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

Issue Details

Issue #53497

Please review

Author: Marusyk
Assignees: -
Labels:

area-System.IO

Milestone: -

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

@Marusyk overall it looks good to me, but some tests are failing. To run the tests locally:

.\build.cmd -rc Release -subset clr+libs+libs.tests
.\.dotnet\dotnet.exe build -c Debug -f net6.0-windows .\src\libraries\System.IO.FileSystem\tests\System.IO.FileSystem.Tests.csproj /t:Test

If you want to run specific test you need to pass the following property:

/p:XunitMethodName=Namespace.TypeName.MehtodName

@stephentoub
Copy link
Member

@adamsitnik, if 0 is now to be considered valid, why do we still have tests like this?

[Theory,
InlineData(0),
InlineData(-1)]
public void InvalidBufferSize_Throws(int size)
{
using (var handle = new SafeFileHandle(new IntPtr(1), ownsHandle: false))
{
AssertExtensions.Throws<ArgumentOutOfRangeException>("bufferSize", () => CreateFileStream(handle, FileAccess.Read, size));
}
}

Thanks.

@adamsitnik
Copy link
Member

@adamsitnik, if 0 is now to be considered valid, why do we still have tests like this?

I've changed that only for FileStream created from path. When I realized that the handle ctor is having the old check I've created #53497 to track this and this PR is about fixing that.

@stephentoub
Copy link
Member

I've created #53497 to track this and this PR is about fixing that.

Ok, but that issue mentions nothing about this public FileStream surface area.

@adamsitnik
Copy link
Member

Ok, but that issue mentions nothing about this public FileStream surface area.

Because I have a fix for that in #53669

@stephentoub
Copy link
Member

Because I have a fix for that in #53669

Ok, then this PR is blocked on that one. The test failures here are because of that.

@adamsitnik
Copy link
Member

In #53669 we have fixed it for FileStream ctors that accept handles. In #53982 (comment) we got to the conclusion that we are not going to change it for StreamReader and StreamWriter.

@Marusyk thank you for your help and apologies for inconvenience from our side

@adamsitnik adamsitnik closed this Jul 1, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 31, 2021
@Marusyk Marusyk deleted the rmarusyk/53497 branch January 15, 2022 16:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants