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

Async-Streams: iteration stops early on Core #31268

Closed
jcouv opened this Issue Nov 19, 2018 · 14 comments

Comments

Projects
None yet
6 participants
@jcouv
Copy link
Member

jcouv commented Nov 19, 2018

Reported by @BillWagner

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.cs from Common.zip
  • add reference to System.Threading.Tasks.Extension
  • run

Expected: number printed up to 99
Actual: only 0 is printed

@jcouv jcouv added this to the 16.0.P2 milestone Nov 19, 2018

@jcouv jcouv self-assigned this Nov 19, 2018

@jcouv

This comment has been minimized.

Copy link
Member

jcouv commented Nov 19, 2018

Tried this on a .NET Core 3.0 project (installed from here) and got the same problem.

@stephentoub

This comment has been minimized.

Copy link
Member

stephentoub commented Nov 19, 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.

@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented Dec 5, 2018

@stephentoub

It's a codegen problem to do with the finally block emitted for the foreach

which foreach are you referring to here?

@BillWagner

This comment has been minimized.

Copy link
Member

BillWagner commented Dec 5, 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);
    }
}
@richlander

This comment has been minimized.

Copy link
Member

richlander commented Dec 5, 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.

@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented Dec 5, 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.

@stephentoub

This comment has been minimized.

Copy link
Member

stephentoub commented Dec 5, 2018

Actually, hmm, I'm looking at it again, and my analysis may have been wrong... re-checking.

@stephentoub

This comment has been minimized.

Copy link
Member

stephentoub commented Dec 5, 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.

@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented Dec 5, 2018

Why is this only an issue on Core then? I would expect such a problem to happen in both desktop and core.

@stephentoub

This comment has been minimized.

Copy link
Member

stephentoub commented Dec 5, 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.

@jcouv

This comment has been minimized.

Copy link
Member

jcouv commented Dec 6, 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: #31527).

Below is an illustration, annotated with comments:

    static async System.Collections.Generic.IAsyncEnumerable<int> M(System.Collections.Generic.IEnumerable<int> collection)
    {
        foreach (var i in collection)
        {
            yield return i;
            await System.Threading.Tasks.Task.Delay(10);
        }
    }
{
    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;
}
@yyjdelete

This comment has been minimized.

Copy link

yyjdelete commented Dec 7, 2018

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;
        }
@jcouv

This comment has been minimized.

Copy link
Member

jcouv commented Dec 10, 2018

@yyjdelete Yes, this is a related issue. try/finally for async-iterator methods will be implemented in preview2.

@jaredpar jaredpar added the Bug label Jan 2, 2019

@jcouv

This comment has been minimized.

Copy link
Member

jcouv commented Jan 8, 2019

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 jcouv closed this Jan 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment