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

Add ReadAtLeastAsync implementation for StreamPipeReader #52246

Merged
merged 14 commits into from
May 10, 2021

Conversation

manandre
Copy link
Contributor

@manandre manandre commented May 4, 2021

Closes #52150

/cc @davidfowl

@davidfowl
Copy link
Member

@manandre This conflicts with your other PR can you resolve those.

@manandre manandre force-pushed the streampipereader-readatleastasync branch from cbe195e to 2126bd8 Compare May 5, 2021 09:44
@manandre
Copy link
Contributor Author

manandre commented May 9, 2021

The net48 build is failing with 5 errors due to the usage of internal System.IO.Pipelines types in its test project. What is the issue here? As only net461 (and net6.0) TFM are supported, where does this build come from?

@davidfowl
Copy link
Member

@ViktorHofer Can you help with the .NET 4.8 issue?

@ViktorHofer
Copy link
Member

ViktorHofer commented May 10, 2021

The net48 build is failing with 5 errors due to the usage of internal System.IO.Pipelines types in its test project. What is the issue here? As only net461 (and net6.0) TFM are supported, where does this build come from?

The pipeline is called net48 but is actually running all .NET Framework tfms. In this case the build failed for net461 because you referenced the reference assembly of System.IO.Pipelines but tried to access internal types that are only exposed in the source assembly. You already changed it to reference the source assembly instead. The changes LGTM.

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

Infra changes LGTM

@davidfowl davidfowl merged commit 2ab0058 into dotnet:main May 10, 2021
@davidfowl
Copy link
Member

@manandre Thanks for the contribution!

@manandre manandre deleted the streampipereader-readatleastasync branch May 10, 2021 16:15
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
// TODO ReadyAsync needs to throw if there are overlapping reads.
ThrowIfCompleted();

cancellationToken.ThrowIfCancellationRequested();
Copy link
Member

Choose a reason for hiding this comment

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

This should really be:

if (cancellationToken.IsCancellationRequested)
{
    return ValueTask.FromCanceled<ReadResult>(cancellationToken);
}

or to be able to target downlevel as well:

if (cancellationToken.IsCancellationRequested)
{
    return new ValueTask<ReadResult>(Task.FromCanceled<ReadResult>(cancellationToken));
}

Otherwise, this is going to propagate the OperationCanceledException synchronously out of the ReadAtLeastAsync call, when we generally prefer to propagate it as part of the returned task.

Copy link
Member

Choose a reason for hiding this comment

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

@manandre would you like to take this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!
Fix proposal available in PR #53306

@ghost ghost locked as resolved and limited conversation to collaborators Jun 25, 2021
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.

Implement ReadAtLeastAsync in StreamPipeReader
5 participants