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

Document System.IO.Pipelines.PipeOptions #3494

Merged
merged 4 commits into from
Nov 19, 2019

Conversation

carlossanlop
Copy link
Member

PR that introduced the System.IO.Pipelines namespace: dotnet/corefx#27007

PipeOptions code: corefx/src/System.IO.Pipelines/src/System/IO/Pipelines/PipeOptions.cs

Area owners: @davidfowl @jkotalik @halter73 @anurse

@carlossanlop carlossanlop added new-content Indicates PRs that contain new articles waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews 🏁 Release: .NET Core 2.x Identifies work items for the .NET Core 2.x releases labels Nov 14, 2019
@carlossanlop carlossanlop added this to the November 2019 milestone Nov 14, 2019
@carlossanlop carlossanlop self-assigned this Nov 14, 2019
@halter73
Copy link
Member

Personally, I would use "async continuations" instead of "callbacks" everywhere "callbacks" are referenced. I would also make sure everywhere thresholds are referenced, we communicate that setting them to a negative value disables them.

Copy link
Contributor

@mairaw mairaw left a comment

Choose a reason for hiding this comment

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

Looks good, left a few comments

xml/System.IO.Pipelines/PipeOptions.xml Outdated Show resolved Hide resolved
xml/System.IO.Pipelines/PipeOptions.xml Outdated Show resolved Hide resolved
xml/System.IO.Pipelines/PipeOptions.xml Outdated Show resolved Hide resolved
xml/System.IO.Pipelines/PipeOptions.xml Outdated Show resolved Hide resolved
xml/System.IO.Pipelines/PipeOptions.xml Outdated Show resolved Hide resolved
xml/System.IO.Pipelines/PipeOptions.xml Outdated Show resolved Hide resolved
xml/System.IO.Pipelines/PipeOptions.xml Outdated Show resolved Hide resolved
Co-Authored-By: Stephen Halter <halter73@gmail.com>
Co-Authored-By: Maira Wenzel <mairaw@microsoft.com>
<param name="readerScheduler">The <see cref="T:System.IO.Pipelines.PipeScheduler" /> to be used to execute <see cref="T:System.IO.Pipelines.PipeReader" /> callbacks and async continuations.</param>
<param name="writerScheduler">The <see cref="T:System.IO.Pipelines.PipeScheduler" /> used to execute <see cref="T:System.IO.Pipelines.PipeWriter" /> callbacks and async continuations.</param>
<param name="pauseWriterThreshold">The number of bytes in the <see cref="T:System.IO.Pipelines.Pipe" /> before <see cref="M:System.IO.Pipelines.PipeWriter.FlushAsync(System.Threading.CancellationToken)" /> starts blocking. A negative value prevents <see cref="M:System.IO.Pipelines.PipeWriter.FlushAsync(System.Threading.CancellationToken)" /> from ever blocking, effectively making the number of bytes in the <see cref="T:System.IO.Pipelines.Pipe" /> unlimited.</param>
<param name="resumeWriterThreshold">The number of bytes in the <see cref="T:System.IO.Pipelines.Pipe" /> when <see cref="M:System.IO.Pipelines.PipeWriter.FlushAsync(System.Threading.CancellationToken)" /> stops blocking.</param>
Copy link
Member Author

Choose a reason for hiding this comment

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

@halter73 you wrote:

Personally, I would use "async continuations" instead of "callbacks" everywhere "callbacks" are referenced. I would also make sure everywhere thresholds are referenced, we communicate that setting them to a negative value disables them.

But I'm not entirely sure how to explain it for resumeWriterThreshold. Would you mind helping here?

Copy link
Member

Choose a reason for hiding this comment

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

This one is a little weird because technically you don't need to disable resumeWriterThreshold if you've already disabled the pauseWriterThreshold. And if the pauseWriterThreshold isn't disabled, it would be silly to disable the resumeWriterThreshold. So keeping this as-is and not mentioning what happens when you set the resumeWriterThreshold negative is probably fine.

@carlossanlop
Copy link
Member Author

@halter73 would you mind taking another quick look? I modified the places where callback was used and included continuations.

@BillWagner BillWagner removed their request for review November 19, 2019 15:41
@carlossanlop carlossanlop added verify-build-before-merge and removed waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews labels Nov 19, 2019
@carlossanlop carlossanlop merged commit 225052c into dotnet:master Nov 19, 2019
@carlossanlop carlossanlop deleted the Pipelines_PipeOptions branch November 19, 2019 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏁 Release: .NET Core 2.x Identifies work items for the .NET Core 2.x releases new-content Indicates PRs that contain new articles
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants