-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Akka:Streams Resolve IAsyncEnumerator.DisposeAsync
bug
#6290
Akka:Streams Resolve IAsyncEnumerator.DisposeAsync
bug
#6290
Conversation
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.
Detailed my changes
@@ -5,6 +5,7 @@ | |||
<PropertyGroup> | |||
<AssemblyName>Akka.Streams.Tests</AssemblyName> | |||
<TargetFrameworks>$(NetFrameworkTestVersion);$(NetTestVersion);$(NetCoreTestVersion)</TargetFrameworks> | |||
<LangVersion>9</LangVersion> |
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 this in order to natively support IAsyncEnumerable
inside both Akka.Streams and its test suite. We can consider dropping the dependencies on the BCL NuGet package that pulls it in now.
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.
<PackageReference Include="Microsoft.Bcl.AsyncInterfaces" Version="5.0.0" />
Is referenced in Akka.Streams - let me see if I can remove it in an additional commit.
foreach (var i in Enumerable.Range(0, 100)) | ||
{ | ||
if (i > 50) | ||
await Task.Delay(1000); |
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.
This is designed to force the ValueTask
returned from IAsyncEnumerator.MoveNextAsync()
to be still running at the moment we attempt to dispose the IAsyncEnumerator
.
var source = Source.From(GenerateInts); | ||
var subscriber = this.CreateManualSubscriberProbe<int>(); | ||
|
||
await EventFilter.Warning().ExpectAsync(0, async () => |
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.
This assertion failed until I implemented my fix.
var subscription = subscriber.ExpectSubscription(); | ||
subscription.Request(50); | ||
subscriber.ExpectNextN(Enumerable.Range(0, 50)); | ||
subscription.Request(10); // the iterator is going to start delaying 1000ms per item here |
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.
Force the IAsyncEnumerable
to emit the elements that start producing 1s delays.
subscription.Request(50); | ||
subscriber.ExpectNextN(Enumerable.Range(0, 50)); | ||
subscription.Request(10); // the iterator is going to start delaying 1000ms per item here | ||
subscription.Cancel(); |
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.
And cancel immediately afterwards - this will trigger the exception before I made my changes.
@@ -3817,19 +3817,7 @@ public override void OnDownstreamFinish() | |||
_completionCts.Cancel(); | |||
_completionCts.Dispose(); | |||
|
|||
try | |||
{ | |||
_enumerator.DisposeAsync().GetAwaiter().GetResult(); |
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 right solution here is to not dispose the iterator - iteration should stop automatically as a result of the CancellationToken
and the DisposeAsync
method won't do anything in scenarios where we're still waiting on the Task
anyway. GC should still clean this up afterwards.
The alternative, adding a bunch of accounting code to safely call DisposeAsync
, is really infeasible since we can't pass the ValueTask
around (because it's a struct
) and only supports a single await
-er - unless we want to wrap it in a Task
each time.
None of our other stages which retain an IEnumerator<T>
call Dispose
on it, so I don't think it's an issue.
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.
Correct, the _completionCts.Token
is passed into the _enumerator
sink during construction, this will gracefully shuts down the _enumerable
on cancellation.
We don't need to call DisposeAsync
here.
…b.com/Aaronontheweb/akka.net into AkkaStreams-IAsyncEnumerable-disposal
No idea why AzDo isn't running.... |
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
@@ -3817,19 +3817,7 @@ public override void OnDownstreamFinish() | |||
_completionCts.Cancel(); | |||
_completionCts.Dispose(); | |||
|
|||
try | |||
{ | |||
_enumerator.DisposeAsync().GetAwaiter().GetResult(); |
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.
Correct, the _completionCts.Token
is passed into the _enumerator
sink during construction, this will gracefully shuts down the _enumerable
on cancellation.
We don't need to call DisposeAsync
here.
Fixes akkadotnet#6280 We no longer attempt to dispose IAsyncEnumerators inside Source stages due to the reasons outlined here: akkadotnet#6280 (comment) This prevents the log from being filled with NotSupportedException warnings from failed DisposeAsync operations. (cherry-picked from 3156272)
…` bug (#6296) * Backports #6290 Fixes #6280 We no longer attempt to dispose IAsyncEnumerators inside Source stages due to the reasons outlined here: #6280 (comment) This prevents the log from being filled with NotSupportedException warnings from failed DisposeAsync operations. (cherry-picked from 3156272) * post-merge fix Co-authored-by: Aaron Stannard <aaron@petabridge.com>
Fixes #6280
Changes
We no longer attempt to dispose
IAsyncEnumerator
s insideSource
stages due to the reasons outlined here: #6280 (comment)This prevents the log from being filled with
NotSupportedException
warnings from failedDisposeAsync
operations.Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):
System.NotSupportedException
when disposing stage with materializedIAsyncEnumerable
#6280