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

Unify BufferedFileStreamStrategy seeking #78984

Merged
merged 6 commits into from
Dec 23, 2022
Merged

Conversation

pedrobsaila
Copy link
Contributor

Fixes #78457

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 29, 2022
@ghost
Copy link

ghost commented Nov 29, 2022

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #78457

Author: pedrobsaila
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.

@stephentoub thanks for pointing this out. To answer your question I've studied our code and realized that most likely this setter is never executed?

FileStream.Postion = value is calling _strategy.Seek instead _strategy.Position = value.

set
{
if (value < 0)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.value, ExceptionResource.ArgumentOutOfRange_NeedNonNegNum);
}
_strategy.Seek(value, SeekOrigin.Begin);
}

My guess is that we are doing so because we actually want it to throw for closed or non-seekable files the same way we do for the getter:

public override long Position
{
get
{
if (_strategy.IsClosed)
{
ThrowHelper.ThrowObjectDisposedException_FileClosed();
}
else if (!CanSeek)
{
ThrowHelper.ThrowNotSupportedException_UnseekableStream();
}
return _strategy.Position;

@pedrobsaila I encourage you to refactor both FileStream.Position { set; } and BufferedFileStreamStrategy.Position { set; }.
If you can simplify the code and avoid similar confusions, we are going to merge the refactor.

Thank you for working on this and apologies for the confusion, I should have done better research when I was creating the issue.

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.

@pedrobsaila thank you for your contribution and appologies for the delay in the review.

I am going to clone your fork and push some minor fixes to it to save the time and get the PR merged today.

@adamsitnik adamsitnik merged commit 3f7222a into dotnet:main Dec 23, 2022
@adamsitnik adamsitnik added this to the 8.0.0 milestone Dec 23, 2022
@build-analysis build-analysis bot mentioned this pull request Dec 23, 2022
@pedrobsaila pedrobsaila deleted the 78457 branch January 16, 2023 19:41
@ghost ghost locked as resolved and limited conversation to collaborators Feb 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unify BufferedFileStreamStrategy seeking
3 participants