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 TextWriter.CreateBroadcasting #96732

Merged
merged 3 commits into from
Jan 22, 2024

Conversation

stephentoub
Copy link
Member

Closes #93623

@stephentoub stephentoub added this to the 9.0.0 milestone Jan 10, 2024
@ghost
Copy link

ghost commented Jan 10, 2024

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

Issue Details

Closes #93623

Author: stephentoub
Assignees: -
Labels:

area-System.IO

Milestone: 9.0.0

Copy link

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, 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.

@ghost ghost assigned stephentoub Jan 10, 2024
@skyoxZ
Copy link
Contributor

skyoxZ commented Jan 10, 2024

Does the behavior or the method name still have a chance to change?

/// The resulting instance will delegate each operation to each of the writers in <paramref name="writers"/>.
/// For example, calling <see cref="Write(char)"/> will write the specified char to each writer, one after the
/// other. The writers will be written to in the same order as they are specified in <paramref name="writers"/>.
/// An exception from the operation on one writer will prevent the operation from being performed on subsequent writers.

For this behavior, Chain seems to be a better name for me. And for the name of broadcasting I would expect the behavior of broadcasting.WriteAsync to be something like Task.WhenAll.

@stephentoub
Copy link
Member Author

For this behavior, Chain seems to be a better name for me.

When I think Chain, I think of it as first writing everything to the first writer, and then when the first writer is full, writing everything to the second writer, etc., rather than writing everything to everything.

And for the name of broadcasting I would expect the behavior of broadcasting.WriteAsync to be something like Task.WhenAll.

Why? Does that mean you'd expect broadcasting.Write would use Parallel.For across all of the writers? I don't see why broadcast implies concurrent execution.

@skyoxZ
Copy link
Contributor

skyoxZ commented Jan 11, 2024

Why? Does that mean you'd expect broadcasting.Write would use Parallel.For across all of the writers? I don't see why broadcast implies concurrent execution.

My intuition tells me that a sync method usually doesn't contain any concurrent operation but it's normal if an async method contains some concurrent operations. Then considering the name of Broadcast, I would be a little surprised its WriteAsync doesn't implement concurrent so the operation takes more time to finish than I expected.

@stephentoub
Copy link
Member Author

stephentoub commented Jan 11, 2024

but it's normal if an async method contains some concurrent operations

It's exceedingly rare for methods in the core libraries to introduce any concurrency local to their operation (eg fork multiple tasks and then join with them). The only one I can think of off the top of my head is in Microsoft.Extensions and is opt-in.

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.

It looks good, but it needs some minor polishing before we merge it. Thank you @stephentoub !

PS. I am using "Request changes" on purpose, as agreed in our Stale PRs discussion. To make others start using it and to treat internal and external PRs equally.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 11, 2024
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 19, 2024
@stephentoub
Copy link
Member Author

@adamsitnik, I addressed your feedback. Please re-review.

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.

LGTM, thank you @stephentoub !

@stephentoub stephentoub merged commit fd45f3b into dotnet:main Jan 22, 2024
175 of 178 checks passed
@stephentoub stephentoub deleted the createbroadcasting branch January 22, 2024 16:33
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
* Add TextWriter.CreateBroadcasting

* Address PR feedback
@github-actions github-actions bot locked and limited conversation to collaborators Feb 22, 2024
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.

[API Proposal]: TextWriter.Aggregate
4 participants