Skip to content

AsyncEnumerable - #6044

Merged
Aaronontheweb merged 12 commits into
akkadotnet:devfrom
eaba:async_enumerable_as_run
Jul 18, 2022
Merged

Aaronontheweb merged 12 commits into
akkadotnet:devfrom
eaba:async_enumerable_as_run

Conversation

@eaba

@eaba eaba commented Jul 13, 2022

Copy link
Copy Markdown
Contributor

Changes

IAsyncEnumerable as a streams source

Comment on lines +3858 to +3860
//TODO: when to dispose async enumerable? Should this be a part of ownership of current stage, or should it
// be a responsibility of the caller?
_asyncEnumerable = asyncEnumerable;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Aaronontheweb :

Not entirely related but Should we be taking a Func<IAsyncEnumerable<T>> here? (In my opinion yes, in ideal-state, a Source itself should be idempotent and able to be materialized multiple times.)

As to this question, for whatever my opinion is worth I'd think that since we are consuming the stream till the end, we should be disposing when we are done whether it is provided by a func producer or live instance.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not entirely related but Should we be taking a Func<IAsyncEnumerable> here? (In my opinion yes, in ideal-state, a Source itself should be idempotent and able to be materialized multiple times.)

Now that is a good question. We should absolutely program this component to accept this Func so we can restart the stream in the event of errors. That'll also make disposal of the IAsyncEnumerable less ambigious too: we created it, we own it, so we clean it up.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eaba can you implement this change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ty_IAsyncEnumerable_Is_Passed_In
 Source: AsyncEnumerableSpec.cs line 163
 Duration: 1.7 sec

Message: 
Failed: Expected a message of type Akka.Streams.TestKit.TestSubscriber+OnComplete, but received {TestSubscriber.OnNext(21)} (type Akka.Streams.TestKit.TestSubscriber+OnNext`1[System.Int32]) instead from [akka://AsyncEnumerableSpec-1/user/StreamSupervisor-0/Flow-0-0-enumerableSource#1834516904]
Expected: True
Actual: False

Stack Trace: 
XunitAssertions.Fail(String format, Object[] args) line 26
d__1361.MoveNext() line 455 --- End of stack trace from previous location where exception was thrown --- TaskAwaiter.ThrowForNonSuccess(Task task) TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) ValueTask1.get_Result()
ConfiguredValueTaskAwaiter.GetResult()
d__1351.MoveNext() line 421 --- End of stack trace from previous location where exception was thrown --- TaskAwaiter.ThrowForNonSuccess(Task task) TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) ValueTask1.get_Result()
ConfiguredValueTaskAwaiter.GetResult()
d__1331.MoveNext() line 389 --- End of stack trace from previous location where exception was thrown --- TaskAwaiter.ThrowForNonSuccess(Task task) TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) ValueTask1.get_Result()
ConfiguredValueTaskAwaiter.GetResult()
d__1311.MoveNext() line 367 --- End of stack trace from previous location where exception was thrown --- TaskAwaiter.ThrowForNonSuccess(Task task) TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) ValueTask1.get_Result()
ConfiguredValueTaskAwaiter.GetResult()
d__1151.MoveNext() line 52 --- End of stack trace from previous location where exception was thrown --- TaskAwaiter.ThrowForNonSuccess(Task task) TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) ValueTask1.get_Result()
ValueTaskAwaiter1.GetResult() <ExpectCompleteTask>d__85.MoveNext() line 117 --- End of stack trace from previous location where exception was thrown --- TaskAwaiter.ThrowForNonSuccess(Task task) TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) ConfiguredTaskAwaiter.GetResult() ManualProbe1.ExpectComplete(CancellationToken cancellationToken) line 186
AsyncEnumerableSpec.AsyncEnumerableSource_Must_Complete_Immediately_With_No_elements_When_An_Empty_IAsyncEnumerable_Is_Passed_In() line 182

Standard Output: 
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
[DEBUG][7/16/2022 2:09:57 PM][Thread 0027][ActorSystem(AsyncEnumerableSpec-1)] System shutdown initiated
[DEBUG][7/16/2022 2:09:57 PM][Thread 0020][EventStream] Shutting down: StandardOutLogger started
[DEBUG][7/16/2022 2:09:57 PM][Thread 0020][EventStream] All default loggers stopped

@Aaronontheweb

Copy link
Copy Markdown
Member

the code in this PR is copied from @Horusiath Akka.Persistence.Pulsar - and while the copyright headers and licenses are identical to Akka.NET's we'll need to obtain his permission before proceeding.

@Horusiath

Copy link
Copy Markdown
Contributor

@Aaronontheweb @eaba I don't mind coping the code. Feel free to use it.

@Aaronontheweb

Copy link
Copy Markdown
Member

thanks @Horusiath - we'll move forward with these changes then.

@Aaronontheweb Aaronontheweb added this to the 1.5.0 milestone Jul 15, 2022
@Aaronontheweb

Copy link
Copy Markdown
Member

Once this is merged we'll need to backport it to the v1.4 branch in order to close #5836

@Aaronontheweb

Copy link
Copy Markdown
Member

Compilation error on Linux...

/home/vsts/work/1/s/src/core/Akka.Streams.Tests/Dsl/AsyncEnumerableSpec.cs(185,22): error CS0433: The type 'IAsyncEnumerable<T>' exists in both 'Microsoft.Bcl.AsyncInterfaces, Version=6.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' and 'mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089' [/home/vsts/work/1/s/src/core/Akka.Streams.Tests/Akka.Streams.Tests.csproj]
    557 Warning(s)
    1 Error(s)

@Aaronontheweb

Copy link
Copy Markdown
Member

Weird that we'd have that problem given that we already have IAsyncEnumerable support elsewhere in Akka.Streams

Comment thread src/core/Akka.Streams.Tests/Dsl/AsyncEnumerableSpec.cs
@Aaronontheweb

Copy link
Copy Markdown
Member

Current test failures are unrelated to your PR I think - looks like mostly Akka.Persistence issues.

@Aaronontheweb Aaronontheweb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

[Akka.Annotations.InternalApiAttribute()]
public sealed class AsyncEnumerable<T> : Akka.Streams.Stage.GraphStage<Akka.Streams.SourceShape<T>>
{
public AsyncEnumerable(System.Func<System.Collections.Generic.IAsyncEnumerable<T>> factory) { }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

}
else
{
vtask.AsTask().ContinueWith(_handleContinuation);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would probably be better with a local function that does await but we can refactor this later.

Comment thread src/core/Akka.Streams/Implementation/Fusing/Ops.cs Outdated
Comment thread src/core/Akka.Streams/Implementation/Fusing/Ops.cs Outdated
@Aaronontheweb
Aaronontheweb enabled auto-merge (squash) July 18, 2022 16:43
@Aaronontheweb
Aaronontheweb disabled auto-merge July 18, 2022 16:43
@Aaronontheweb
Aaronontheweb enabled auto-merge (squash) July 18, 2022 16:43
@Aaronontheweb
Aaronontheweb merged commit af513b0 into akkadotnet:dev Jul 18, 2022
@eaba
eaba deleted the async_enumerable_as_run branch July 18, 2022 17:17
Aaronontheweb pushed a commit that referenced this pull request Jul 22, 2022
* Cherry-picked from af513b0

* Fix missing DefaultConfig

* Fix missing StreamTestDefaultMailbox.DefaultConfig

Co-authored-by: Ebere Abanonu <eaba@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

4 participants