Skip to content
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: Call AsyncIteratorMethodBuilder.Complete #39436

Merged
merged 4 commits into from Oct 22, 2019

Conversation

@jcouv
Copy link
Member

jcouv commented Oct 21, 2019

Fixes #39321

We need to call AsyncIteratorMethodBuilder.Complete to dispose of some resources it holds. We can do that when bringing the state machine to a finalized state (-2), that is when we'd call AsyncTaskMethodBuilder.SetResult in a plain async method.
But a completed instance cannot be re-used, so we also need to reset it in GetAsyncEnumerator.

For more context, here's the code we currently produce.

Here's a sample showing an overview of the change:

	private sealed class <M>d__0 : IAsyncEnumerable<int>, IAsyncEnumerator<int>, IAsyncDisposable, IValueTaskSource<bool>, IValueTaskSource, IAsyncStateMachine
	{
		public int <>1__state;
		public AsyncIteratorMethodBuilder <>t__builder;
		public ManualResetValueTaskSourceCore<bool> <>v__promiseOfValueOrEnd;
		private int <>2__current;
		private bool <>w__disposeMode;
		private int <>l__initialThreadId;
		private TaskAwaiter <>u__1;

		[DebuggerHidden]
		public <M>d__0(int <>1__state)
		{
			this.<>1__state = <>1__state;
			<>l__initialThreadId = Environment.CurrentManagedThreadId;
                        <>t__builder = AsyncIteratorMethodBuilder.Create();
		}

		private void MoveNext()
		{
			int num = <>1__state;
			try
			{
                            ...
			}
			catch (Exception exception)
			{
				<>1__state = -2;
				<>v__promiseOfValueOrEnd.SetException(exception);
				<>t__builder.Complete(); // added
				return;
			}
			<>1__state = -2;
			<>v__promiseOfValueOrEnd.SetResult(result: false);
			<>t__builder.Complete(); // added
			return;
			IL_011f:
			<>v__promiseOfValueOrEnd.SetResult(result: true);
		}

		[DebuggerHidden]
		IAsyncEnumerator<int> IAsyncEnumerable<int>.GetAsyncEnumerator(CancellationToken token)
		{
			if (<>1__state == -2 && <>l__initialThreadId == Environment.CurrentManagedThreadId)
			{
				<>1__state = -3;
				<>t__builder = AsyncIteratorMethodBuilder.Create(); // added
				<>w__disposeMode = false;
				return this;
			}
			return new <M>d__0(-3);
		}

            ...
	}
@jcouv jcouv added this to the 16.4.P3 milestone Oct 21, 2019
@jcouv jcouv self-assigned this Oct 21, 2019
@jcouv jcouv force-pushed the jcouv:builder-complete branch 2 times, most recently from f235b4f to 2dcb6dc Oct 21, 2019
@jcouv jcouv marked this pull request as ready for review Oct 21, 2019
@jcouv jcouv requested a review from dotnet/roslyn-compiler as a code owner Oct 21, 2019
@jcouv jcouv force-pushed the jcouv:builder-complete branch from 2dcb6dc to 2670518 Oct 21, 2019
@jcouv jcouv force-pushed the jcouv:builder-complete branch from 2d1a90c to 2d9b5bb Oct 22, 2019
@jcouv

This comment has been minimized.

Copy link
Member Author

jcouv commented Oct 22, 2019

Rebased to resolve conflict #Resolved

@jcouv

This comment has been minimized.

Copy link
Member Author

jcouv commented Oct 22, 2019

@dotnet/roslyn-compiler Please review. I'd like to get this fix in before tomorrow's snap. Thanks #Resolved

Copy link
Member

333fred left a comment

:shipit:

IL_0134: ldarg.0
IL_0135: ldflda ""System.Threading.Tasks.Sources.ManualResetValueTaskSourceCore<bool> C.<M>d__0.<>v__promiseOfValueOrEnd""
IL_013a: ldc.i4.1
IL_013b: call ""void System.Threading.Tasks.Sources.ManualResetValueTaskSourceCore<bool>.SetResult(bool)""

This comment has been minimized.

Copy link
@agocke

agocke Oct 22, 2019

Contributor

Why don't we need to call AsyncIteratorMethodBuilder.Complete here? #Resolved

This comment has been minimized.

Copy link
@jcouv

jcouv Oct 22, 2019

Author Member

The tail of the MoveNext() method is like this:

            // ... _exprReturnLabel: ...
            // ... this.state = FinishedState; ...
            // if (this.combinedTokens != null) { this.combinedTokens.Dispose(); this.combinedTokens = null; } // for enumerables only
            // this.promiseOfValueOrEnd.SetResult(false);
            // this.builder.Complete();
            // return;

            // _exprReturnLabelTrue:
            // this.promiseOfValueOrEnd.SetResult(true);

            // ... _exitLabel: ...
            // ... return; ...

It has different ways of returning from MoveNext() (in non-exceptional flow):

  • one for signaling that we have no values left: we set the finished state, dispose our CancellationTokenSource, record that we're done with SetResult(false) and dispose the builder with Complete().
  • the other for returning a value in the middle of the stream: we have already save that value in Current, we just signal with SetResult(true). We don't need to dispose any resources, since we expect to process more values in the stream.

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

This comment has been minimized.

Copy link
@jcouv

jcouv Oct 22, 2019

Author Member

Also it helps to look at the example I linked to in description showing current tail of MoveNext(). Only two of the exit paths need to dispose.

            }
            catch (Exception exception)
            {
                <>1__state = -2;
                <>v__promiseOfValueOrEnd.SetException(exception);
                return;
            }
            <>1__state = -2;
            <>v__promiseOfValueOrEnd.SetResult(false);
            return;
            IL_00ed:
            <>v__promiseOfValueOrEnd.SetResult(true);
        }

In reply to: 337681134 [](ancestors = 337681134,337657566)

This comment has been minimized.

Copy link
@agocke

agocke Oct 22, 2019

Contributor

Ah, I see, thanks

@@ -102,6 +104,19 @@ BoundExpressionStatement generateSetResultOnPromise(bool result)
}
}

BoundExpressionStatement GenerateCompleteOnBuilder()

This comment has been minimized.

Copy link
@cston

cston Oct 22, 2019

Member

B [](start = 8, length = 1)

private #Resolved

BoundExpressionStatement GenerateCompleteOnBuilder()
{
// Produce:
// this.builder.Complete();

This comment has been minimized.

Copy link
@cston

cston Oct 22, 2019

Member

this.builder.Complete(); [](start = 15, length = 24)

It looks like the code below is builder.SetResult(value). #WontFix

_asyncMethodBuilderMemberCollection.SetResult, // AsyncIteratorMethodBuilder.Complete is the corresponding method to AsyncTaskMethodBuilder.SetResult
_method.IsGenericTaskReturningAsync(F.Compilation)
? ImmutableArray.Create<BoundExpression>(F.Local(_exprRetValue))
: ImmutableArray<BoundExpression>.Empty));

This comment has been minimized.

Copy link
@cston

cston Oct 22, 2019

Member

Is there an argument? #Resolved

This comment has been minimized.

Copy link
@jcouv

jcouv Oct 22, 2019

Author Member

You're correct that I re-used this call from a call to SetResult. I should have removed the argument (SetResult never has an argument). Thanks!


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

// this.builder = System.Runtime.CompilerServices.AsyncVoidMethodBuilder.Create();
AsyncMethodBuilderMemberCollection methodScopeAsyncMethodBuilderMemberCollection;
bool found = AsyncMethodBuilderMemberCollection.TryCreate(F, method, typeMap: null, out methodScopeAsyncMethodBuilderMemberCollection);
Debug.Assert(found);

This comment has been minimized.

Copy link
@cston

cston Oct 22, 2019

Member

The base class has an _asyncMethodBuilderMemberCollection field. Consider using that here rather than calling TryCreate() again which does extra work (and might produce duplicate warnings?). Same comment for GenerateConstructor() above. #Resolved

@@ -133,7 +133,7 @@ internal static bool TryCreate(SyntheticBoundNodeFactory F, MethodSymbol method,
createBuilderMethod: createBuilderMethod,
taskProperty: null,
setException: null, // unused
setResult: WellKnownMember.System_Runtime_CompilerServices_AsyncIteratorMethodBuilder__Complete,
setResult: WellKnownMember.System_Runtime_CompilerServices_AsyncIteratorMethodBuilder__Complete, // AsyncIteratorMethodBuilder.Complete is the corresponding method to AsyncTaskMethodBuilder.SetResult

This comment has been minimized.

Copy link
@agocke

agocke Oct 22, 2019

Contributor

I find this kind of a confusing pattern, but I'm not sure how I would change it #ByDesign

This comment has been minimized.

Copy link
@jcouv

jcouv Oct 22, 2019

Author Member

The BCL initially had AsyncIteratorMethodBuilder.SetResult (like other builders), but then changed the name since there is no value/result. Still is it the spiritual equivalent... ;-)


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

null,
methodScopeAsyncMethodBuilderMemberCollection.CreateBuilder)));
// this.builder = System.Runtime.CompilerServices.AsyncIteratorMethodBuilder.Create();
bodyBuilder.Add(GenerateCreateAndAssignBuilder());

bodyBuilder.Add(F.Return());
F.CloseMethod(F.Block(bodyBuilder.ToImmutableAndFree()));
bodyBuilder = null;

This comment has been minimized.

Copy link
@cston

cston Oct 22, 2019

Member

bodyBuilder = null; [](start = 16, length = 19)

Why assign null?

This comment has been minimized.

Copy link
@jcouv

jcouv Oct 22, 2019

Author Member

This was from a previous code review if I recall correctly. I don't care much, but it does prevent accidental re-use of a free'd pooled instance.

@@ -51,7 +51,7 @@ internal class AsyncMethodToStateMachineRewriter : MethodToStateMachineRewriter
/// of rewritten return expressions. The return-handling code then uses <c>SetResult</c> on the async method builder
/// to make the result available to the caller.
/// </summary>
private readonly LocalSymbol _exprRetValue;
protected readonly LocalSymbol _exprRetValue;

This comment has been minimized.

Copy link
@cston

cston Oct 22, 2019

Member

protected [](start = 8, length = 9)

Is this change needed?

@cston
cston approved these changes Oct 22, 2019
@agocke
agocke approved these changes Oct 22, 2019
IL_0134: ldarg.0
IL_0135: ldflda ""System.Threading.Tasks.Sources.ManualResetValueTaskSourceCore<bool> C.<M>d__0.<>v__promiseOfValueOrEnd""
IL_013a: ldc.i4.1
IL_013b: call ""void System.Threading.Tasks.Sources.ManualResetValueTaskSourceCore<bool>.SetResult(bool)""

This comment has been minimized.

Copy link
@agocke

agocke Oct 22, 2019

Contributor

Ah, I see, thanks

@jcouv jcouv changed the title Call AsyncIteratorMethodBuilder.Complete Async-streams: Call AsyncIteratorMethodBuilder.Complete Oct 22, 2019
@jcouv jcouv merged commit 0e7b657 into dotnet:master Oct 22, 2019
18 checks passed
18 checks passed
WIP Ready for review
Details
license/cla All CLA requirements met.
Details
roslyn-CI Build #20191022.11 succeeded
Details
roslyn-CI (Linux_Test coreclr) Linux_Test coreclr succeeded
Details
roslyn-CI (SourceBuild_Test) SourceBuild_Test succeeded
Details
roslyn-CI (Windows_CoreClr_Unit_Tests debug) Windows_CoreClr_Unit_Tests debug succeeded
Details
roslyn-CI (Windows_CoreClr_Unit_Tests release) Windows_CoreClr_Unit_Tests release succeeded
Details
roslyn-CI (Windows_Correctness_Test) Windows_Correctness_Test succeeded
Details
roslyn-CI (Windows_Desktop_Spanish_Unit_Tests) Windows_Desktop_Spanish_Unit_Tests succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests debug_32) Windows_Desktop_Unit_Tests debug_32 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests debug_64) Windows_Desktop_Unit_Tests debug_64 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests release_32) Windows_Desktop_Unit_Tests release_32 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests release_64) Windows_Desktop_Unit_Tests release_64 succeeded
Details
roslyn-CI (Windows_Determinism_Test) Windows_Determinism_Test succeeded
Details
roslyn-CI (macOS_Test) macOS_Test succeeded
Details
roslyn-integration-CI Build #20191022.11 had test failures
Details
roslyn-integration-CI (VS_Integration debug_async) VS_Integration debug_async succeeded
Details
roslyn-integration-CI (VS_Integration release_async) VS_Integration release_async succeeded
Details
@jcouv

This comment has been minimized.

Copy link
Member Author

jcouv commented Oct 22, 2019

FYI @stephentoub Fixed in VS 16.4p3.

@jcouv jcouv deleted the jcouv:builder-complete branch Oct 22, 2019
@stephentoub

This comment has been minimized.

Copy link
Member

stephentoub commented Oct 22, 2019

Thanks!

@dgrunwald dgrunwald mentioned this pull request Oct 31, 2019
dgrunwald added a commit to icsharpcode/ILSpy that referenced this pull request Nov 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.