Add IAsyncEnumerable support to TransformManyBlock

[manandre]

Contributes to #30863

cc @onionhammer @stephentoub

[dotnet-issue-labeler]

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.

[ghost]

Tagging subscribers to this area:
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Contributes to #30863

cc @onionhammer @stephentoub

Author:	manandre
Assignees:	-
Labels:	area-System.Threading.Tasks, new-api-needs-documentation
Milestone:	-

[jeffhandley]

Tagging @eiriktsarpalis as he might be interested in reviewing this.

[jeffhandley]

Tagging @adamsitnik, @carlossanlop, and @jozkee for reviewing this.

[jeffhandley]

Pinging @adamsitnik, @carlossanlop, and @jozkee for a review on this.

[jeffhandley]

@adamsitnik This PR is assigned to you for follow-up/decision before the RC1 snap.

[adamsitnik]

what are the benefits of changing private ctor to a method? You need to pass few more extra args, take references and use ! to pretend that fields which are nulls are not nulls. I don't like this part of the refactor

[manandre]

It is an attempt to solve an issue reported by @stephentoub during a previous review in #37876 (comment)
To duplicate code or not to duplicate code... that is the question!

[adamsitnik]

@stephentoub could you please review this PR as well? I don't feel confident signing off DataFlow PRs yet.

[carlossanlop]

@manandre The PR description says "contributes to" instead of "fixes". Does that mean there will still be some pending work to address after you merge this, or is this the last PR to fully address the issue?

[stephentoub]

could you please review this PR as well?

Yes. I need to set aside some time for it... I'm still a bit hesitant around all the code duplication.

[manandre]

@manandre The PR description says "contributes to" instead of "fixes". Does that mean there will still be some pending work to address after you merge this, or is this the last PR to fully address the issue?

It should be the last PR to complete the IAsyncEnumerable support in Dataflow.

[adamsitnik]

FWIW I've tried to wrap my head around the additional duplication and the only idea I had was to use map Task<IEnumerable<TOutput>> to IAsyncEnumerable<TOutput> and use IAsyncEnumerable everywhere.

Pseudocode:

private async IAsyncEnumerable<TOutput> Map(Task<IEnumerable<TOutput>> oldWay)
{
    foreach (TOutput item in await oldWay.ConfigureAwait(false))
    {
        yield return item;
    }
}

The obvious disadvantage would be the perf hit, but the real blocker is that DataFlow targets monikers like net461 where IAsyncEnumerable does not exist.

Based on #30863 (comment) I've moved it to 7.0.

[jozkee]

@manandre any thoughts on the code duplication issue pointed out by @adamsitnik and @stephentoub?

[jeffhandley]

@manandre Let us know if you'd be able to try the approach @stephentoub described. Thanks!

[manandre]

If we want to implement the TransformManyBlock over its "IAsyncEnumerable-based transform" implementation, we will have to address multiple issues:

• netstandard2.1 (minimum) is required, then support of net461 and netstandard2.0 TFMs must be dropped from the System.Threading.Tasks.Dataflow library. Is it possible?
• the internal TargetCore class cannot be used without doing sync-over-async as its "callAction" callback is synchronous, and writing a new AsyncTargetCore would also lead to a lot of duplicated code :/
• impacting the existing IEnumerable<T> and Task<IEnumerable<T>> implementations could introduce some significative performance regressions, what does not seem recommended on such a perf-critical framework like Dataflow...

I am still ready to contribute for the full support of IAsyncEnumerable in Dataflow, but, I would need some help to address these issues before starting any new implementation.

[buyaa-n]

• netstandard2.1 (minimum) is required, then support of net461 and netstandard2.0 TFMs must be dropped from the System.Threading.Tasks.Dataflow library. Is it possible?

Normally we would if-def the target platforms, but i guess that would cause again a code dup which we want to avoid, @joperezr mentioned we could not drop those TFMs, what we should do here @stephentoub?

• the internal TargetCore class cannot be used without doing sync-over-async as its "callAction" callback is synchronous, and writing a new AsyncTargetCore would also lead to a lot of duplicated code :/

From the SslStream implementation that abstracting sync over async read and write that @stephentoub metnioned above doesn't require everything to be async for a real, for example:

runtime/src/libraries/System.Net.Security/src/System/Net/Security/ReadWriteAdapter.cs

Lines 52 to 59 in ce15dc7

	public ValueTask<int> ReadAsync(Memory<byte> buffer) =>
	new ValueTask<int>(_stream.Read(buffer.Span));
	public ValueTask WriteAsync(byte[] buffer, int offset, int count)
	{
	_stream.Write(buffer, offset, count);
	return default;
	}

• impacting the existing IEnumerable<T> and Task<IEnumerable<T>> implementations could introduce some significative performance regressions, what does not seem recommended on such a perf-critical framework like Dataflow...

For me that really depend on implementation and not have to inroduce perf regression, @manandre have you tried it and got some perf results?

[ghost]

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

[stephentoub]

This is still on me to spend some time on and try to come up with a better approach.

[deeprobin]

LGTM
I just added a few small code style notes and I would like to see some comments on the test flow so that the tests can be understood when roughly reading over them.

[ghost]

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

[deeprobin]

@stephentoub Whats the current status of this?

[stephentoub]

@deeprobin, why are you asking about status on this PR?

[deeprobin]

Because I have planned to look at the DataFlow APIs in a private project and would like to consume this API.

[stephentoub]

My plan is for it to be in .NET 7.

[stephentoub]

Over the last few weeks I experimented with various approaches, but there's enough variation between the three IEnumerable<T>, Task<IEnumerable<T>>, and IAsyncEnumerable<T> code paths that it's difficult to share meaningfully. I fixed a few issues in the PR, e.g. we shouldn't be synchronously blocking in the IAsyncEnumerable<T> code path, we should be ensuring work is run on the right scheduler, etc. I also added some more tests.

@manandre, thank you very much for working on this, and thank you for your patience.

stale