Amortize StreamPipeReader ReadAsync calls

[benaadams]

Current

PR

Resolves https://github.com/dotnet/corefx/issues/39258

[benaadams]

Raised issue for Json failure https://github.com/dotnet/corefx/issues/39451

[davidfowl]

Another concern I have is test coverage, how many tests this new code path?

[benaadams]

Another concern I have is test coverage, how many tests this new code path?

Is quite covered.

Not covered:

Flow EC for Competion trigger; throw for not reading on completion (should never happen)

Zero read and unknown exception

[benaadams]

That's with adding new test to check for throws on concurrent reads

[benaadams]

@joperezr would this be a correct change? Were some R2R issues with this package previously for ASP.NET that were fixed with #38731

ASP.NET now currently only targets netcoreapp3.0 and I was having packaging issues trying include a reference for ManualResetValueTaskSourceCore until I reached this current state. While it complies and packages, is it correct for ASP.NET's packaging?

[joperezr]

mm I'm not sure why you would want to do this change, even if ASP.NET is targeting netcoreapp3.0, if they reference the package they would still get the netcoreapp2.0 asset resolved, which is what you want here right? I don't have full context on this change but I don't think doing this is correct. cc: @ericstj

[benaadams]

Would want ASP.NET to resolve the netcoreapp3.0 asset; don't want a netcoreapp2.0 asset as it needs the extra downlevel Microsoft.Bcl.AsyncInterfaces assembly

[joperezr]

it won't be a netcoreapp2.0 asset, it will be a netstandard reference assembly and it will be the same one you will get no matter how you package it. If you really want to not depend on AsyncInterfaces, then you would need to cross compile the reference assembly for netcoreapp3.0. What this line here is doing, is simply telling our infrastructure to grab the assembly that targets netstandard2.0 and package it as if it was cross-compiled for netcoreapp2.0, but it still has the same dependencies. What you really want here, is to add a netcoreapp3.0 target on the configurations.props of this ref project so that we actually target netcoreapp3.0 and won't have the dependency. Does that make sense?

[joperezr]

If you prefer, I'm happy to push a change directly in your PR to fix this configuration issue and to make sure that the package is built correctly and that you won't get AsyncInterfaces dependency in ASP.NET.

[benaadams]

I'd love that! :)

[joperezr]

Done, that last changed I pushed should fix this and produce the right package.

[danmoseley]

@stephentoub I assume this is post 3.0?

[stephentoub]

From my perspective, yes.

[davidfowl]

I love this change but want to figure out the bcl.async thing.

[benaadams]

I love this change but want to figure out the bcl.async thing.

Think is fixed now?

[maryamariyan]

Commenting to revive this PR. Seems like this was halted just as a matter of waiting post 3.0.

cc: @anurse @jkotalik as area owners and @benaadams as PR creator

[benaadams]

Packaging is still not happy with it :(

[ericstj]

D:\a\1\s\.dotnet\sdk\3.0.100-preview7-012630\Microsoft.Common.CurrentVersion.targets(2106,5): error MSB3245: Could not resolve this reference. Could not locate the assembly "System.Threading.Tasks.Extensions". Check to make sure the assembly exists on disk. If this reference is required by your code, you may get compilation errors. [D:\a\1\s\src\System.IO.Pipelines\ref\System.IO.Pipelines.csproj]

Reference assemblies need to use ProjectReferences in most cases. Only exceptions are for frameworks that aren't building any more (eg: past NETCoreApps and NETStandards), in which case we've disabled those configurations and restore them from past packages.

[benaadams]

Probably want to hold on this for now until dotnet/coreclr#26310 is evaluated

[davidfowl]

Gonna close @benaadams