Skip to content

Add an option to do zero byte reads on StreamPipeReader #49117

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 11, 2021
Merged

Add an option to do zero byte reads on StreamPipeReader #49117

merged 1 commit into from
Mar 11, 2021

Conversation

davidfowl
Copy link
Member

@davidfowl davidfowl commented Mar 4, 2021 •
edited
Loading

  • Added UseZeroByteReads to StreamPipeReader options that allows not allocating a buffer by doing a zero byte read on the underlying Stream before the internal buffer is allocated.

Fixes #37539

TODO: Finish API review

@davidfowl davidfowl added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 4, 2021
@ghost
Copy link

ghost commented Mar 4, 2021

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.

jkotas
jkotas reviewed Mar 4, 2021
View reviewed changes
jkotas
jkotas reviewed Mar 4, 2021
View reviewed changes
jkotas
jkotas reviewed Mar 4, 2021
View reviewed changes
stephentoub
stephentoub reviewed Mar 4, 2021
View reviewed changes
@davidfowl
Add an option to do zero byte reads on StreamPipeReader
65959ec 
- Added WaitForData to StreamPipeReader options that allows not allocating a buffer by doing a zero byte read on the underlying Stream before the internal buffer is allocated.
@davidfowl davidfowl force-pushed the davidfowl/wait-for-data branch from 4b16f10 to 65959ec Compare March 10, 2021 00:12
@davidfowl davidfowl removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 10, 2021
@davidfowl
Copy link
Member Author

This is ready for review.

BrennanConroy
BrennanConroy approved these changes Mar 10, 2021
View reviewed changes
src/libraries/System.IO.Pipelines/tests/StreamPipeReaderTests.cs
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
[InlineData(false)]
[InlineData(true)]
public async Task CanReadMultipleTimes(bool useZeroByteReads)
Copy link
Member

Choose a reason for hiding this comment

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

There should be another test with a mocked stream so we can check that it actually issues zero byte reads

Copy link
Member

Choose a reason for hiding this comment

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

😢

@davidfowl
Copy link
Member Author

Something broken with Codedom?

@stephentoub
Copy link
Member

stephentoub commented Mar 10, 2021 •
edited
Loading

Not sure about codedom, but I've been seeing this error on a variety of PRs (including this one) over the last day or so:
"Validation Failed: System.Text.Encodings.Web is harvesting assets from package version 5.0.0 which is not the latest for that package release. Latest package version from that release is 5.0.1. In order to fix this, run `dotnet msbuild System.Text.Encodings.Web.pkgproj"
@ViktorHofer, @ericstj, known issue?

@ViktorHofer
Copy link
Member

@ericstj fixed that issue yesterday after new packages were published to nuget.org.

@stephentoub
Copy link
Member

fixed that issue yesterday after new packages were published to nuget.org.

This failure was only 3 hrs ago. Was that somehow based on something stale?

@BrennanConroy
Copy link
Member

The last push was 22 hours ago, if you re-run from the AzDo UI I don't think you get a fresh rebase on main, so you're still running against 22 hour old bits. If you use /azp run then it will get a fresh rebase.

@davidfowl
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

@davidfowl
Copy link
Member Author

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@davidfowl
Copy link
Member Author

/azp run runtime (Libraries Build windows allConfigurations x64 Debug)

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@davidfowl
Copy link
Member Author

/azp run runtime windows allConfigurations x64 Debug

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@davidfowl davidfowl merged commit 82ca681 into main Mar 11, 2021
@davidfowl davidfowl deleted the davidfowl/wait-for-data branch March 11, 2021 07:49
@davidfowl davidfowl mentioned this pull request Mar 12, 2021
Merged
@runfoapp runfoapp bot mentioned this pull request Mar 22, 2021
Closed
@davidfowl davidfowl mentioned this pull request Mar 31, 2021
Closed
@davidfowl davidfowl added this to the 6.0.0 milestone Apr 2, 2021
@ghost ghost locked as resolved and limited conversation to collaborators May 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@jkotas jkotas jkotas left review comments

@BrennanConroy BrennanConroy BrennanConroy approved these changes

@ericstj ericstj ericstj left review comments

@halter73 halter73 Awaiting requested review from halter73

Assignees
No one assigned
Labels
area-System.IO.Pipelines
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

Add ability to perform zero byte reads in StreamPipeReader
6 participants
@davidfowl @stephentoub @ViktorHofer @BrennanConroy @jkotas @ericstj