Add guard to DisposeAsync by jcouv · Pull Request #31764 · dotnet/roslyn

jcouv · 2018-12-13T20:17:57Z

The behavior for DisposeAsync is only specified if the method is at the beginning, the end or a yield return. It is unspecified if the method is suspended at an await or running.
Per discussion with LDM, although it doesn't not guarantee that users couldn't call DisposeAsync at the wrong time (there are still possible race conditions that we can't guard against) we'll throw in such cases.

To that effect:

a new state is introduced to represent the beginning of the method (-3)
the states for yield return and await are split: yield return uses negative numbers (-4 and down), await uses positive numbers (0 and up, as in regular async methods)

The APIs could be mis-used in other ways , but we will not guard those:

calling MoveNextAsync() on two separate threads is unspecified,
calling MoveNextAsync() after DisposeAsync() is unspecified, but the implementation just returns false (no items left).

ValueTask IAsyncDisposable.DisposeAsync()
{
    if (state >= StateMachineStates.NotStartedStateMachine /* -1 */)
    {
        throw new NotSupportedException();
    }
    if (state == StateMachineStates.FinishedStateMachine /* -2 */)
    {
        return default;
    }
    disposeMode = true;
    _valueOrEndPromise.Reset();
    var inst = this;
    _builder.Start(ref inst);
    return new ValueTask(this, _valueOrEndPromise.Version);  // note this leverages the state machine's implementation of IValueTaskSource
}

        DisposeAsync                              await
 +------------------------+             +------------------------> N
 |                        |             |                          |
 v   GetAsyncEnumerator   |             |        resuming          |
-2 --------------------> -3 --------> -1 <-------------------------+    Dispose mode = false
 ^                                   |  |                          |
 |         done and disposed         |  |      yield return        |
 +-----------------------------------+  +-----------------------> -N
 |                                   |                             |
 |                                   |                             |
 |                             yield |                             |
 |                             break |           DisposeAsync      |
 |                                   |  +--------------------------+
 |                                   |  |
 |                                   |  |
 |         done and disposed         v  v    suspension (await)
 +----------------------------------- -1 ------------------------> N
                                        ^                          |    Dispose mode = true
                                        |         resuming         |
                                        +--------------------------+

Fixes #30109

agocke · 2018-12-19T22:11:43Z

src/Compilers/CSharp/Portable/Lowering/StateMachineRewriter/StateMachineRewriter.cs

            {
                managedThreadId = MakeCurrentThreadId();
+
+                var thenBuilder = ArrayBuilder<BoundStatement>.GetInstance();


Give the size, since it's known statically. #Resolved

agocke · 2018-12-19T22:19:12Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs

    }
 }");
-            CompileAndVerify(comp);
+            CompileAndVerify(comp, verify: Verification.Skipped);


Why does this fail verification? #Resolved

My bad. Forgot to undo this

In reply to: 243095742 [](ancestors = 243095742)

agocke · 2018-12-19T22:23:16Z

docs/features/async-streams.md


 When in dispose mode, MoveNext continues to suspend (N) and resume (-1) until the end of the method is reached (-2).

+The result of invoking `DisposeAsync` from states -1 or N is unspecified. This compiler throws `NotSupportException` for those cases.


What about calling DisposeAsync simultaneously on multiple threads? Undefined? #Resolved

Correct. That's also unspecified #Resolved

jaredpar · 2019-01-02T16:35:39Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs

                );
        }

+        [ConditionalFact(typeof(WindowsDesktopOnly))]


What bug is tracking this being enabled on CoreClr again? #Resolved

#30438

In reply to: 244785283 [](ancestors = 244785283)

jaredpar · 2019-01-02T16:37:15Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs

+        yield return 1;
+        while (true)
+        {
+            await Task.Delay(10);


Why not Task.Yield? #Resolved

Habit and didn't know about Yield(). The main goal is to avoid the optimization for already completed tasks.

In reply to: 244785798 [](ancestors = 244785798)

Understood. The problem with Delay is that over the totality of our unit tests it ends up causing a noticable performance issue. At least historically it's done that. #Resolved

jaredpar · 2019-01-02T16:37:33Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs

+            token.ThrowIfCancellationRequested();
+        }
+    }
+    public static async System.Threading.Tasks.Task Main()


Why the fully qualified name here? #Resolved

jaredpar · 2019-01-02T16:38:30Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs

+        {
+            await task;
+        }
+        catch (System.OperationCanceledException) { }


Consider logging that the operation was cancelled successfully. #Resolved

jaredpar · 2019-01-02T16:41:30Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs

+    {
+        var enumerator = M().GetAsyncEnumerator();
+        await enumerator.DisposeAsync();
+        System.Console.Write(""done"");


Consider a test where you call MoveNext after DisposeAsync. I did not see one in the file (may have missed it). #Resolved

I think that is covered by DisposeAsyncTwiceAfterRunning below.

In reply to: 244787127 [](ancestors = 244787127)

jaredpar · 2019-01-02T16:45:18Z

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncRewriter.AsyncIteratorRewriter.cs

            private readonly bool _isEnumerable;

+            // We use state=-3 to distinguish the initial state from the running state (-1)
+            private const int InitialState = -3;


-3; [](start = 45, length = 3)

The -3 constant is used twice in this PR. Consider adding a named constant to StateMachinesState. #Resolved

jaredpar · 2019-01-02T16:47:48Z

...ompilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncIteratorMethodToStateMachineRewriter.cs

+        /// <summary>
+        /// States for `yield return` are decreasing from -3.
+        /// </summary>
+        private int _nextYieldReturnState = StateMachineStates.FinishedStateMachine - 1;  // -3


StateMachineStates.FinishedStateMachine - 1; // -3 [](start = 43, length = 52)

This is another location where we could use a named constant for -3 #Resolved

jaredpar · 2019-01-02T16:49:31Z

Done with review pass (iteration #2) #Resolved

jaredpar

jcouv · 2019-01-04T17:28:54Z

@agocke Did you have any more feedback? Thanks #Resolved

cston · 2019-01-04T18:12:17Z

docs/features/async-streams.md


 When in dispose mode, MoveNext continues to suspend (N) and resume (-1) until the end of the method is reached (-2).

+The result of invoking `DisposeAsync` from states -1 or N is unspecified. This compiler throws `NotSupportException` for those cases.


This compiler throws [](start = 74, length = 20)

Perhaps "The compiler generates throw NotSupportedException ..." or "The generated code throws NotSupportedException ..." #Pending

Definitely. I'll include this fix in my next async-streams PR.

In reply to: 245376450 [](ancestors = 245376450)

cston · 2019-01-04T18:20:19Z

...ompilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncIteratorMethodToStateMachineRewriter.cs


+        /// <summary>
+        /// Lower the body, adding an entry state (-3) at the start,
+        /// so that we can differentiate a async-iterator that was never moved forward with MoveNextAsync()


a [](start = 41, length = 1)

an? #Pending

I'll include this fix in my next async-streams PR.

In reply to: 245378700 [](ancestors = 245378700)

agocke

LGTM

cston · 2019-01-04T18:39:15Z

src/Compilers/CSharp/Portable/Lowering/StateMachineRewriter/StateMachineRewriter.cs

+                    F.Assignment(F.Local(resultVariable), F.This()));
+
+                var extraReset = GetExtraResetForIteratorGetEnumerator();
+                if (extraReset != null)


extraReset != null [](start = 20, length = 18)

Would it make sense to pass in the builder to GetExtraResetForIteratorGetEnumerator()? (That would also allow adding multiple statements, although we probably don't have such a scenario today.) #WontFix

That can be refactored easily when the need arises.

In reply to: 245383654 [](ancestors = 245383654)

cston · 2019-01-04T18:49:15Z

src/Compilers/CSharp/Portable/Lowering/StateMachineRewriter/StateMachineRewriter.cs

+                thenBuilder.Add(
+                    method.IsStatic || method.ThisParameter.Type.IsReferenceType ? // if this is a reference type, no need to copy it since it is not assignable
+                        F.Goto(thisInitialized) : // goto thisInitialized
+                        (BoundStatement)F.StatementList());


(BoundStatement)F.StatementList() [](start = 24, length = 33)

Why add an empty statement list? #Pending

I don't know. That's existing code though, so I'll leave as-is given the deadline.

In reply to: 245386255 [](ancestors = 245386255)

I'll fix in next PR

In reply to: 245392475 [](ancestors = 245392475,245386255)

jcouv added Area-Compilers Feature - Async Streams Async Streams labels Dec 13, 2018

jcouv added this to the 16.0.P2 milestone Dec 13, 2018

jcouv self-assigned this Dec 13, 2018

jcouv requested a review from a team as a code owner December 13, 2018 20:17

Add guard to DisposeAsync

f77189d

jcouv force-pushed the guard-dispose branch from 926fca9 to f77189d Compare December 13, 2018 20:30

jcouv mentioned this pull request Dec 13, 2018

[Umbrella] Work items and test plan for async streams #24037

Closed

agocke reviewed Dec 19, 2018

View reviewed changes

Address feedback from Andy

46f18fb

jaredpar reviewed Jan 2, 2019

View reviewed changes

Address feedback from Jared

2a42d25

jaredpar approved these changes Jan 2, 2019

View reviewed changes

cston reviewed Jan 4, 2019

View reviewed changes

agocke approved these changes Jan 4, 2019

View reviewed changes

cston reviewed Jan 4, 2019

View reviewed changes

jcouv merged commit 966dcfc into dotnet:master Jan 4, 2019

jcouv deleted the guard-dispose branch January 4, 2019 19:21


		When in dispose mode, MoveNext continues to suspend (N) and resume (-1) until the end of the method is reached (-2).

		The result of invoking `DisposeAsync` from states -1 or N is unspecified. This compiler throws `NotSupportException` for those cases.

Conversation

jcouv commented Dec 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agocke Dec 19, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agocke Dec 19, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agocke Dec 19, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Dec 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Jan 2, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Jan 2, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Jan 2, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Jan 2, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Jan 2, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Jan 2, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Jan 2, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Jan 2, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Jan 2, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredpar left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston Jan 4, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jan 4, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jcouv commented Dec 13, 2018 •

edited

Loading

agocke Dec 19, 2018 •

edited by jcouv

Loading

agocke Dec 19, 2018 •

edited by jcouv

Loading

agocke Dec 19, 2018 •

edited by jcouv

Loading

jcouv Dec 20, 2018 •

edited

Loading

jaredpar Jan 2, 2019 •

edited by jcouv

Loading

jaredpar Jan 2, 2019 •

edited by jcouv

Loading

jaredpar Jan 2, 2019 •

edited by jcouv

Loading

jaredpar Jan 2, 2019 •

edited by jcouv

Loading

jaredpar Jan 2, 2019 •

edited by jcouv

Loading

jaredpar Jan 2, 2019 •

edited by jcouv

Loading

jaredpar Jan 2, 2019 •

edited by jcouv

Loading

jaredpar Jan 2, 2019 •

edited by jcouv

Loading

jaredpar commented Jan 2, 2019 •

edited by jcouv

Loading

jcouv commented Jan 4, 2019 •

edited

Loading

cston Jan 4, 2019 •

edited by jcouv

Loading

cston Jan 4, 2019 •

edited by jcouv

Loading

cston Jan 4, 2019 •

edited by jcouv

Loading

cston Jan 4, 2019 •

edited by jcouv

Loading