Roslyn: Async-Streams: iteration stops early on Core
Reported by @BillWagner
```C#
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
namespace AsyncStream
{
class Program
{
static async Task Main(string[] args)
{
var asyncSequence = Numbers();
//var enumerator = asyncSequence.GetAsyncEnumerator();
//while (await enumerator.MoveNextAsync())
// Console.WriteLine(enumerator.Current);
await foreach (int num in asyncSequence)
Console.WriteLine(num);
}
private static async IAsyncEnumerable<int> Numbers()
{
foreach (var i in Enumerable.Range(0, 100))
{
yield return i;
await Task.Delay(100);
}
}
}
}
```
Repro steps:
- create a Core program
- add this source
- add
Common.csfrom Common.zip - add reference to
System.Threading.Tasks.Extension - run
Expected: number printed up to 99
Actual: only 0 is printed
jcouv
All 14 comments
Tried this on a .NET Core 3.0 project (installed from here) and got the same problem.
jcouv
on 19 Nov 2018
It's a codegen problem to do with the finally block emitted for the foreach (if you change the foreach to instead just get the enumerator from Range and use MoveNext/Current, the problem goes away). The finally block ends up disposing of the enumerator before it actually should. The first time we enter, the state is -1, and that's stored into a local. Before yielding, we update the state to no longer be -1, but the finally block when it checks is checking against the local, not the updated state.
stephentoub
on 19 Nov 2018
@stephentoub
It's a codegen problem to do with the finally block emitted for the foreach
which foreach are you referring to here?
jaredpar
on 5 Dec 2018
@jaredpar
This one:
private static async IAsyncEnumerable<int> Numbers()
{
foreach (var i in Enumerable.Range(0, 100))
{
yield return i;
await Task.Delay(100);
}
}
In a .NET Core project, that method must be written as follows:
private static async IAsyncEnumerable<int> Numbers()
{
var e = Enumerable.Range(0,100).GetEnumerator();
while (e.MoveNext())
{
yield return e.Current;
await Task.Delay(100);
}
}
BillWagner
on 5 Dec 2018
Should we tell users to skip this feature until preview 2? Probably all that would be needed is to change the sample in the blog post.
richlander
on 5 Dec 2018
Should we tell users to skip this feature until preview 2?
This is a preview, not RTM. If we told users to skip preview features because they had bugs we should just stop shipping previews altogether. 馃槃
The blog post by Mads is a bit different here and is possibly walking around this bug. Once we have the root cause we can see if the blog post is affected. If so yeah we should definitely update it.
jaredpar
on 5 Dec 2018
Actually, hmm, I'm looking at it again, and my analysis may have been wrong... re-checking.
stephentoub
on 5 Dec 2018
Nope, I was right the first time :)
<>1__state is initialized to -1 in Numbers(). When we enter <Numbers>d__1.MoveNext, we store <>1__state into a local num; that'll be -1. We then do the Enumerable.Range(0, 100).GetEnumerator() and call MoveNext() and get its Current. We update <>1__state to 0, but we fail to update num to be 0, too. We then return, causing us to go through the finally block. The finally block checks whether the local num is < 0, which it still will be, and calls Dispose on the enumerator if it was negative.
stephentoub
on 5 Dec 2018
Why is this only an issue on Core then? I would expect such a problem to happen in both desktop and core.
jaredpar
on 5 Dec 2018
Why is this only an issue on Core then? I would expect such a problem to happen in both desktop and core.
Because in .NET Core, the enumerable returned by Enumerable.Range sets its _state to -1 as part of its Dispose:
https://source.dot.net/#System.Linq/System/Linq/Range.cs,69
In .NET Framework, Enumerable.Range is implemented as an iterator, and its Dispose is a nop:
https://referencesource.microsoft.com/#System.Core/System/Linq/Enumerable.cs,1271
(To preempt the next question, I don't know why the Dispose in core isn't a nop as well... seems like it could or maybe should be to increase compat, though in reality you shouldn't be using this after you dispose it.)
So this compiler bug that calls Dispose impacts .NET Core more in this specific case. It would effect them equally for any other foreach'd enumerable inside of an async enumerator that has logic in its Dispose, e.g. looping over something with a finally.
stephentoub
on 5 Dec 2018
Just to confirm Stephen's analysis: the issue arises when you use yield return in a construct that has a finally (such as a try/finally, a foreach or await foreach, or using or await using) and the Dispose method actually does work.
The problem is that the yield return will trigger the Dispose method to run too early.
The fix for this will be in preview2 (implementation of disposal and try/finally behavior in async-iterators: https://github.com/dotnet/roslyn/pull/31527).
Below is an illustration, annotated with comments:
```C#
static async System.Collections.Generic.IAsyncEnumerable
{
foreach (var i in collection)
{
yield return i;
await System.Threading.Tasks.Task.Delay(10);
}
}
```C#
{
int temp1;
temp1 = this.<>1__state;
try
{
switch (temp1)
{
case 0:
case 1:
goto <tryDispatch-42>;
break;
}
{
{
{
this.<>7__wrap1 = this.collection.GetEnumerator();
{
<tryDispatch-42>: ;
try
{
switch (temp1)
{
case 0:
goto <stateMachine-39>;
break;
case 1:
goto <stateMachine-40>;
break;
}
goto <continue-34>;
<start-35>: ;
{
(Local System.Int32 i);
(Local System.Int32 i) = this.<>7__wrap1.get_Current();
{
// yield return i;
{
this.<>2__current = (Local System.Int32 i);
this.<>1__state = 0; // <------ this doesn't set the cached state (`temp1`)
this.<>v__promiseOfValueOrEnd.SetResult(True);
goto <exitLabel-38>;
<stateMachine-39>: ;
}
{
// await System.Threading.Tasks.Task.Delay(10);
System.Runtime.CompilerServices.TaskAwaiter temp2;
{
temp2 = Task.Delay(10).GetAwaiter();
{
if (! BoolLogicalNegation temp2.get_IsCompleted()) goto <afterif-41>;
{
this.<>1__state = temp1 = 1;
this.<>u__1 = temp2;
{ temp3 = this; this.<>t__builder.AwaitUnsafeOnCompleted(temp2, temp3) };
goto <exitLabel-38>;
<stateMachine-40>: ;
temp2 = this.<>u__1;
this.<>u__1 = default;
this.<>1__state = temp1 = -1;
}
<afterif-41>: ;
}
}
temp2.GetResult();
}
}
}
<continue-34>: ;
if (this.<>7__wrap1.MoveNext()) goto <start-35>;
<break-33>: ;
}
finally
{
// disposal of enumerator
{
if (temp1 >= 0) goto <afterif-46>; // <------ so this finally isn't getting skipped
if (!Conversion
NotEqual null) goto <afterif-36>;
Conversion
.Dispose();
<afterif-36>: ;
<afterif-46>: ;
}
}
}
}
this.<>7__wrap1 = null;
}
goto <exprReturn-37>;
}
}
catch (Exception)
{
this.<>1__state = -2;
this.<>v__promiseOfValueOrEnd.SetException(temp4);
goto <exitLabel-38>;
}
<exprReturn-37>: ;
this.<>1__state = -2;
this.<>v__promiseOfValueOrEnd.SetResult(False);
<exitLabel-38>: ;
return;
}
Is this the same issue? The finally block is never hit if no await between the first yield and finally
public static async IAsyncEnumerable<int> Test001(bool needThrow)
{
yield return 1;
try
{
//await Task.Delay(100);
Console.WriteLine("TRY-THROW " + needThrow);
if (needThrow)
throw new Exception("test");
}
/*catch
{
//await Task.Delay(100);
Console.WriteLine("CATCH");
throw;
}*/
finally
{
//await Task.Delay(100);
Console.WriteLine("FINALLY");
}
yield return 2;
}
yyjdelete
on 7 Dec 2018
@yyjdelete Yes, this is a related issue. try/finally for async-iterator methods will be implemented in preview2.
jcouv
on 10 Dec 2018
Verified that the issue is fixed in latest Core preview2 SDK (which includes a recent preview2 C# compiler), 3.0.100-preview-010024. The program outputs all the way up to 99 as expected.
I'll go ahead and close the issue.
jcouv
on 8 Jan 2019
Related issues
ashmind
路
3Comments
codingonHP
路
3Comments
nlwolf
路
3Comments
asvishnyakov
路
3Comments
binki
路
3Comments