Add initial async iterators support to System.Private.CoreLib

[stephentoub]

• Adds the IAsyncEnumerable<T>, IAsyncEnumerator<T>, and IAsyncDisposable interfaces
• Adds the AsyncIteratorMethodBuilder struct for the compilers
• Adds IAsyncDisposable implementations to the types in System.Private.CoreLib that benefit from it
• Adds the ManualResetValueTaskSourceLogic type that the compilers use to implement async iterators.
• Adds a few helpers on ExecutionContext and ThreadPool to support ManualResetValueTaskSourceLogic (these helpers are currently internal but could potentially be public in the future).

Contributes to https://github.com/dotnet/corefx/issues/32640
Contributes to https://github.com/dotnet/corefx/issues/32665
Contributes to https://github.com/dotnet/corefx/issues/32664

All of this still needs unit tests and to be exposed in the refs in corefx (this will not be merged until I have that ready and validated locally). There will also be additional work in corefx to complete the above issues, e.g. more Stream-derived types that'll override DisposeAsync. And all of this is still subject to an upcoming API review. I'm putting it up now to get a jump on everything so we can get it merged as soon as possible.

cc: @jcouv, @terrajobst, @jaredpar, @MadsTorgersen, @benaadams, @kouvel

[jcouv]

IAsyncEnumerable [](start = 21, length = 16)

I don't know what level of xml doc is expected. Consider adding (here and other public types).

[stephentoub]

Sure, I'll add 'em.

[jcouv]

MoveNext [](start = 20, length = 8)

While I agree that MoveNext is clearer, I'm now wondering if keeping Start as a name might be better, to keep consistency with previous builders.

[stephentoub]

I understand the desire for consistency, but this is actually one of the things that highlights the conceptual difference between the builders. Start makes sense for Async{Void/Task/Task<T>}MethodBuilder because it's invoked once at the beginning of the operation in order to start it, and it's never used again for the duration of the async method's operation. In contrast, this method on AsyncIteratorMethodBuilder is used every time the compiler needs to push the state machine forward; seems wrong to me to name a different concept the same thing.

[jcouv]

A couple of questions/throughts:

1. previously ValueTask was distributed via Tasks.Extensions package. But IAsyncDisposable depends on ValueTask. Did ValueTask move down, or will IAsyncDisposable live in that assembly/package (or some other assembly/package above it)?
2. we should coordinate on when those types RTM. Is master the proper branch to achieve the timing we need? I want to make sure that those types are only made available as "pre-release" (meaning they can be changed) until we jointly lock things down.
3. I'll need to adjust compiler to depend on AsyncIteratorMethodBuilder, but I won't be able to do that in dev16 preview1 timeframe. I'll look into it when coming back from vacation. I expect the code change is easy. For the test change, I see that the implementation you have is currently a thin wrapper on top of AsyncTaskMethodBuilder, so (unless I missed something) that should be easy too.

[stephentoub]

Did ValueTask move down, or will IAsyncDisposable live in that assembly/package (or some other assembly/package above it)?

ValueTask shipped in System.Private.CoreLib / the System.Runtime contract in .NET Core 2.1. It was then also available in the System.Threading.Tasks.Extensions NuGet package downlevel.

we should coordinate on when those types RTM. Is master the proper branch to achieve the timing we need? I want to make sure that those types are only made available as "pre-release" (meaning they can be changed) until we jointly lock things down.

The master branch is synonymous right now with .NET Core 3.0. Nothing in master will ship before .NET Core 3.0 unless explicit action is taken on it.

I'll need to adjust compiler to depend on AsyncIteratorMethodBuilder, but I won't be able to do that in dev16 preview1 timeframe.

Since it's just depending on AsyncTaskMethodBuilder today, there shouldn't be an issue... you can update the compiler to depend on AsyncIteratorMethodBuilder once this is merged, and then work with that. After preview1 sounds fine. It's also possible we could say "forget AsyncIteratorMethodBuilder, just use AsyncTaskMethodBuilder"... that can obviously be made to work. It just felt like this was different enough that we should have a dedicated API for it.

[jcouv]

Sounds good to me. Thanks

It's also possible we could say "forget AsyncIteratorMethodBuilder, just use AsyncTaskMethodBuilder"...

If AsyncIteratorMethodBuilder were the only new type, I'd be tempted to either rely on AsyncTaskMethodBuilder only, or maybe implement some fallback strategy, in an effort to increase the availability/adoption of the feature.
But since we also need some other types, like MRVTSL, I think AsyncIteratorMethodBuilder makes sense.

[jcouv]

ManualResetValueTaskSourceLogic [](start = 18, length = 31)

It looks like MRVTSL no longer needs to be constructed with an IStrongBox<T> parent.
I'll take a note to update the generate state machine (no longer needs to implement IStrongBox<T>).

[stephentoub]

Right. It was there previously to avoid an allocation in two situations:

• When invoking the continuation with an ExecutionContext
• When queueing the continuation to a SynchronizationContext

I addressed the first with an internal helper on ExecutionContext. I decided for the second that it was better to just incur the allocation when a SynchronizationContext was involved (right now it incurs it on each continuation invocation, but we could change that to allocate once and cache), as this generally isn't a scenario that needs to be optimized heavily.

I really disliked my IStrongBox<MRVTSL> solution here previously because it ended up exposing implementation details out of anything that wanted to wrap an MRVTSL: anyone could cast to the interface and then extract a reference to this critical private state of the implementation.

[jaredpar]

The try / finally in DisposeAsyncCore suggests exceptions are possible. Won't it violate the async dispose pattern to have exceptions escape here vs. returned in ValueTask?

[stephentoub]

Hmm, I'm not quite following you. No exceptions should escape here: if any occur inside DisposeAsyncCore, they'll be stored into the ValueTask that's returned, which presumably is the behavior we'd want.

I have been debating with myself the larger question about whether exceptions should be valid at all. In general Dispose isn't supposed to throw, and thus DisposeAsync shouldn't either (or return a ValueTask that might fault with an exception), but in cases where an implementation's Dispose is already throwing, it seems reasonable to keep parity on the DisposeAsync, rather than eating the exceptions in cases where they wouldn't be eaten in Dispose.

Opinion?

[jaredpar]

Ugg ... I missed that DisposeAsyncCore is async. I only checked this method for it.

Yeah I agree that no exceptions is a great principal but reality doesn't always line up with it.

[jcouv]

Reset [](start = 20, length = 5)

Should Reset guard against calls at certain times (when previous operation isn't completed)?

[stephentoub]

My current thinking is "no", but I'm happy to be convinced otherwise.

Since this is a "manual reset" type, I think the owner of the type should be fully responsible for deciding when they're ok with calling Reset. There are multiple definitions of whether the previous operation is completed, e.g. whether SetResult/Exception have been called, whether GetResult has been called, etc., but it could also be that the consumer of the type has their own scheme in place. We do validation in the IValueTaskSource-implementation methods, because that's about how this instance is consumed when wrapped in a ValueTask, but I've avoided doing such second-guessing in the methods that the implementer uses directly (namely SetResult/Exception and Reset).

[stephentoub]

@dotnet-bot test this please

[stephentoub]

@AndyAyersMS, this is hitting an assert in the JIT / crossgen in some of the legs, e.g.
https://ci.dot.net/job/dotnet_coreclr/job/master/job/arm64_cross_debug_windows_nt_innerloop_prtest/2229/consoleText

Assert failure(PID 8460 [0x0000210c], Thread: 2236 [0x08bc]): Assertion failed 'impTreeLast != nullptr' in 'SyncTextWriter:DisposeAsync():struct:this' (IL size 12)

    File: d:\j\workspace\arm64_cross_d---ffc60a4b\src\jit\importer.cpp Line: 562
    Image: D:\j\workspace\arm64_cross_d---ffc60a4b\bin\Product\Windows_NT.arm64.Debug\x64\crossgen.exe

It aborted on several unix legs, which might be due to the same thing?
https://ci.dot.net/job/dotnet_coreclr/job/master/job/x64_checked_ubuntu_corefx_innerloop_prtest/3372/consoleText

./build.sh: line 428: 34771 Aborted                 (core dumped) $__CrossGenExec /Platform_Assemblies_Paths $__BinDir/IL $__IbcTuning /out $__BinDir/System.Private.CoreLib.dll 

[AndyAyersMS]

Could be the same issue, but might not be. Let me take a look.

[AndyAyersMS]

For the assert: the jit is trying to handle the impact of adding the monitor exit call to a synchronized method that also returns a struct type, and it's not prepared for quite this complicated of a tree: in particular that the source side is a comma.

fgMorphTree BB02, stmt 7 (after)
               [000011] -AC-G+------              *  RETURN    struct
               [000044] -----+------              |     /--*  LCL_VAR   struct V05 tmp4
               [000047] --C-G+------              |  /--*  COMMA     struct
               [000040] --C-G+------              |  |  \--*  CALL help void   HELPER.CORINFO_HELP_MON_EXIT
               [000036] L----+------ arg1 in x1   |  |     +--*  ADDR      long
               [000035] ----G+-N----              |  |     |  \--*  LCL_VAR   ubyte (AX) V03 tmp2
               [000037] -----+------ arg0 in x0   |  |     \--*  LCL_VAR   ref    V00 this
               [000045] -AC-G+------              \--*  COMMA     struct
               [000010] -----+-N----                 |  /--*  LCL_VAR   struct V02 tmp1
               [000043] -A---+------                 \--*  ASG       struct (copy)
               [000041] D----+-N----                    \--*  LCL_VAR   struct V05 tmp4

Only happens selectively because it is ABI dependent. For instance on x64 windows we return the structure via a hidden byref and so returns have a different tree shape.

This is a jit bug that we'll have to fix. Let me see if I can work up a simple repro.

cc @dotnet/jit-contrib

[AndyAyersMS]

Crossgen failure looks to be the same issue (opened as #20499).

Assert failure(PID 29056 [0x00007180], Thread: 29056 [0x7180]): Assertion failed 'impTreeLast != nullptr' in 'SyncTextWriter:DisposeAsync():struct:this' (IL size 12)

    File: /home/andy/repos/coreclr/src/jit/importer.cpp Line: 562
    Image: /home/andy/repos/coreclr/bin/Product/Linux.x64.Debug/crossgen

[stephentoub]

Thanks, @AndyAyersMS.

[stephentoub]

@dotnet-bot test Ubuntu x64 Checked CoreFX Tests please

   System.IO.Tests.File_Delete.Unix_NonExistentPath_Nop [FAIL]
      System.IO.DirectoryNotFoundException : Could not find a part of the path '/tmp/File_Delete_zfohkp2m.0ib/Unix_NonExistentPath_Nop_118_5f6ff699/C'.
      Stack Trace:
            at System.IO.FileSystem.DeleteFile(String fullPath)
            at System.IO.File.Delete(String path)
         /root/corefx-1949935/src/System.IO.FileSystem/tests/File/Delete.cs(15,0): at System.IO.Tests.File_Delete.Delete(String path)
         /root/corefx-1949935/src/System.IO.FileSystem/tests/File/Delete.cs(119,0): at System.IO.Tests.File_Delete.Unix_NonExistentPath_Nop()

[AndyAyersMS]

Other PRs are hitting this ubuntu failure too: #20507.

[stephentoub]

Thanks.

[benaadams]

Should QueueUserWorkItemCallback<TState> use this overload?

[stephentoub]

Should QueueUserWorkItemCallback use this overload?

Don't know... it would need to be measured.

[stephentoub]

@dotnet-bot test Ubuntu x64 Checked CoreFX Tests please