-
Notifications
You must be signed in to change notification settings - Fork 4k
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: Disposal in async-iterator methods #31527
Conversation
// if (State == StateMachineStates.FinishedStateMachine) | ||
TypeSymbol returnType = IAsyncEnumerableOfElementType_MoveNextAsync.ReturnType.TypeSymbol; | ||
|
||
GetPartsForStartingMachine(returnType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetPartsForStartingMachine [](start = 16, length = 26)
📝 extracted a number of locals to this helper method, so that it could be re-used in implementation of DisposeAsync()
. #Resolved
/// finally blocks in case the state machine is disposed. The Dispose method computes the new state | ||
/// and then runs MoveNext. Not used if !this.useFinalizerBookkeeping. | ||
/// </summary> | ||
protected Dictionary<int, int> finalizerStateMap = new Dictionary<int, int>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
finalizerStateMap [](start = 39, length = 17)
📝 removed as it was unused (only added to, but never read from) #Resolved
@@ -56,7 +56,9 @@ async IAsyncEnumerable<int> GetValuesFromServer() | |||
} | |||
``` | |||
|
|||
**open issue**: Design async LINQ | |||
Just like in iterator methods, there are several restrictions on where a yield statement can appear in async-iterator methods: | |||
- It is a compile-time error for a `yield` statement (of either form) to appear in the `finally` clause of a `try` statement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yield
statement (of either form) [](start = 35, length = 34)
Minor point: perhaps just "... a yield return
or yield break
..." #WontFix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those two lines were copied from the existing C# spec. I'll keep as-is.
In reply to: 240409214 [](ancestors = 240409214)
docs/features/async-streams.md
Outdated
|
||
In summary, disposal of an async-iterator works based on four design elements: | ||
- `yield return` (jumps to finally when resuming in dispose mode) | ||
- `yield break` (enters dispose mode and jumpps to finally) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jumpps [](start = 41, length = 6)
Typo #Closed
docs/features/async-streams.md
Outdated
The caller of an async-iterator method should only call `DisposeAsync()` when the method completed or was suspended by a `yield return`. | ||
`DisposeAsync` sets a flag on the state machine ("dispose mode") and (if the method wasn't completed) resumes the execution from the current state. | ||
The state machine can resume execution from a given state (even those located within a `try`). | ||
When the execution is resumed in dispose mode, it jump straight to the relevant `finally`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jump [](start = 50, length = 4)
jumps? #Closed
docs/features/async-streams.md
Outdated
`DisposeAsync` sets a flag on the state machine ("dispose mode") and (if the method wasn't completed) resumes the execution from the current state. | ||
The state machine can resume execution from a given state (even those located within a `try`). | ||
When the execution is resumed in dispose mode, it jump straight to the relevant `finally`. | ||
`finally` blocks may involve pauses and resumes, but only for `await` expressions. As a result of the restrictions imposed on `yield return` (described above), dispose mode never runs into that statement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that statement [](start = 189, length = 14)
What does that statement
refer to? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/features/async-streams.md
Outdated
`finally` blocks may involve pauses and resumes, but only for `await` expressions. As a result of the restrictions imposed on `yield return` (described above), dispose mode never runs into that statement. | ||
Once a `finally` block completes, the execution in dispose mode jumps to the next relevant `finally`, or the end of the method once we reach the top-level. | ||
|
||
Reaching a `yield break` also sets the dispose mode flag and jumps to the next relevant `finally` (or end of the method). By the time we return control to the caller (completing the promise as `false` by reaching the end of the method) all disposal was completed, and the state machine is left in finished state. So `DisposeAsync()` has no work left to do. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By [](start = 122, length = 2)
Consider breaking long line. #Closed
docs/features/async-streams.md
Outdated
// resuming from state=<next_state> will dispatch execution to this label | ||
<next_state_label>: ; | ||
this.state = cachedState = NotStartedStateMachine; | ||
if (disposeMode) /* jump to current finally or exit */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
current [](start = 28, length = 7)
enclosing
? #Closed
docs/features/async-streams.md
Outdated
// resuming from state=<next_state> will dispatch execution to this label | ||
<next_state_label>: ; | ||
this.state = cachedState = NotStartedStateMachine; | ||
if (disposeMode) /* jump to current finally or exit */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* [](start = 17, length = 2)
Minor point: perhaps //
#WontFix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've used the following convention: /* ... */
as a placeholder for some omitted code, but use // ...
to comment on code that is there.
In reply to: 240709778 [](ancestors = 240709778)
docs/features/async-streams.md
Outdated
A `yield break` is lowered as: | ||
```C# | ||
disposeMode = true; | ||
/* jump to current finally or exit */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// jump to enclosing ...
Same comment for other case below. #Closed
@@ -1651,6 +1651,11 @@ protected virtual void VisitFinallyBlock(BoundStatement finallyBlock, ref TLocal | |||
VisitStatement(finallyBlock); // this should generate no pending branches | |||
} | |||
|
|||
public override BoundNode VisitExtractedFinallyBlock(BoundExtractedFinallyBlock node) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BoundExtractedFinallyBlock [](start = 61, length = 26)
Doesn't this node only existing in lowering? If so, this visit method should throw ExceptionUtilities.Unreachable;
. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Async rewriting uses data flow analysis (in StateMachineRewriter.Rewrite
at line 106, we do IteratorAndAsyncCaptureWalker.Analyze
on the incoming body
). At that stage (after local rewriting and async exception handler rewriting), we have extracted finally blocks.
In reply to: 240715273 [](ancestors = 240715273)
/// We use _exprReturnLabel for normal end of method (ie. no more values). | ||
/// We use _exprReturnLabelTrue for `yield return;`. | ||
/// </summary> | ||
private LabelSymbol _exprReturnLabelTrue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private [](start = 8, length = 7)
readonly
#Closed
/// When we enter a `try` that has a `finally`, we'll use the label directly preceeding the `finally`. | ||
/// When we enter a `try` that has an extracted `finally`, we will use the label preceeding the extracted `finally`. | ||
/// </summary> | ||
private LabelSymbol _currentFinallyOrExitLabel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
current [](start = 29, length = 7)
Perhaps enclosing
rather than current
. #Closed
thenClause: F.Goto(_currentFinallyOrExitLabel)); | ||
} | ||
|
||
BoundStatement AppendJumpToCurrentFinallyOrExit(BoundStatement node) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BoundStatement [](start = 8, length = 14)
private
#Closed
BoundStatement AppendJumpToCurrentFinallyOrExit(BoundStatement node) | ||
{ | ||
// Append: | ||
// if (disposeMode) /* jump to current finally or exit */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* jump to current finally or exit */ [](start = 33, length = 37)
goto finallyOrExitLabel;
Same comment for other instances. #Closed
|
||
/// <summary> | ||
/// Initially, this is the method's return value label (<see cref="AsyncMethodToStateMachineRewriter._exprReturnLabel"/>). | ||
/// When we enter a `try` that has a `finally`, we'll use the label directly preceeding the `finally`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The label "directly [preeceding] (sic) the finally" -- is that a label at the end of the try block? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That depends:
- If the
finally
was not extracted, then the label is the last thing in thetry
block. SeeVisitTryStatement
(which adds this label) - If the
finally
was extracted, then the label is after thetry
statement and before the extracted block. (that label is added byAsyncExceptionHandlerRewriter
and saved inBoundTryStatement.FinallyLabelOpt
)
In reply to: 240822167 [](ancestors = 240822167)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Produce: | ||
// disposeMode = true; | ||
// if (state == StateMachineStates.FinishedStateMachine || | ||
// state == StateMachineStates.NotStartedStateMachine) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dispose is supposed to be idempotent. If someone calls it before starting enumeration, then starts enumeration, then disposes, does that still hold? Or is that considered implementation-defined behavior. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this implementation is idempotent already: you can safely call it twice.
I'll add tests for that in my next PR, which also related to DisposeAsync (adding guards).
But I think you should not enumerate after you disposed. I took a note to confirm that (in #24037)
In reply to: 240824375 [](ancestors = 240824375)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found the answer: https://github.com/dotnet/csharplang/blob/master/spec/classes.md#the-dispose-method
if you dispose before enumerating, the state is changed to "after" (ie. end of the method). I'll tweak implementation to reflect that in next PR.
In reply to: 240839838 [](ancestors = 240839838,240824375)
}"; | ||
var comp = CreateCompilationWithAsyncIterator(source, options: TestOptions.DebugExe); | ||
comp.VerifyDiagnostics(); | ||
CompileAndVerify(comp, expectedOutput: "B1::F;D::F;B1::F;"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to have tests that uses goto
to jump out of the try
block. Especially one that re-enters, like
public class C {
public IEnumerable M() {
start:
try{
yield return 0;
goto start;
}
finally
{
}
}
} #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. I also have a work item to do more exploratory testing on goto/continue/break.
In reply to: 240828761 [](ancestors = 240828761)
|
||
MethodSymbol IAsyncDisposable_DisposeAsync = | ||
F.WellKnownMethod(WellKnownMember.System_IAsyncDisposable__DisposeAsync) | ||
.AsMember(IAsyncDisposable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.AsMember(IAsyncDisposable) [](start = 20, length = 27)
Unnecessary. #Closed
.AsMember(IAsyncDisposable); | ||
MethodSymbol IValueTaskSource_GetResult = | ||
F.WellKnownMethod(WellKnownMember.System_Threading_Tasks_Sources_IValueTaskSource__GetResult) | ||
.AsMember(IValueTaskSource); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.AsMember(IValueTaskSource) [](start = 20, length = 27)
Unnecessary. #Closed
F.CloseMethod(F.Block(bodyBuilder.ToImmutableAndFree())); | ||
MethodSymbol IValueTaskSource_OnCompleted = | ||
F.WellKnownMethod(WellKnownMember.System_Threading_Tasks_Sources_IValueTaskSource__OnCompleted) | ||
.AsMember(IValueTaskSource); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.AsMember(IValueTaskSource) [](start = 20, length = 27)
Unnecessary. #Closed
Perhaps And these look like nested blocks rather than top-level. #Closed Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs:2710 in d17fca1. [](commit_id = d17fca1, deletion_comment = False) |
It's an error to have a In reply to: 446482362 [](ancestors = 446482362) Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs:3583 in d17fca1. [](commit_id = d17fca1, deletion_comment = False) |
The name of the test isn't great, but I wanted to have just an await in the In reply to: 446480617 [](ancestors = 446480617) Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs:3220 in d17fca1. [](commit_id = d17fca1, deletion_comment = False) |
But shouldn't the error be In reply to: 446732459 [](ancestors = 446732459,446482362) Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs:3583 in d17fca1. [](commit_id = d17fca1, deletion_comment = False) |
Added a test below. We indeed report the problem of In reply to: 446749399 [](ancestors = 446749399,446732459,446482362) Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs:3583 in d17fca1. [](commit_id = d17fca1, deletion_comment = False) |
@agocke I addressed all the feedback so far. Please take another look. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Overview of the change:
AsyncIteratorMethodToStateMachineRewriter
, instead of mixed inAsyncMethodToStateMachineRewriter
)VisitTryStatement
, which includes adding jumps after finallysAsyncExceptionHandlerRewriter
to wrap extractedfinally
blocks withBoundExtractedFinallyBlock
VisitYieldReturnStatement
andVisitYieldBreakStatement
with logic to jump to relevantfinally
BoundTryStatement
to record the label for extractedfinally
blocksDisposeAsync()
methodIValueTaskSource
to state machine (which previously implementedIValueTaskSource<T>
Please read the design doc (updated in this PR) for more details.
Fixes #30260
Note: I didn't test continue/break/goto yet (tracked in #24037)
Here's an example of lowering (input and pseudo-C# output):
Async-streams umbrella: #24037