-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Akka.Streams: Have SelectAsyncUnordered use local async function instead of ContinueWith
#7531
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: Have SelectAsyncUnordered use local async function instead of ContinueWith
#7531
Conversation
…ntinueWith` Replicates some of the behaviors and fixes we made to `SelectAsync` in akkadotnet#7521
|
More than likely I am going to need to update some unit tests |
Aaronontheweb
left a comment
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
| TryPull(_stage.In); | ||
| return; | ||
|
|
||
| async Task RunTask(Task<TOut> tt) |
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.
Use an async local function with an await on the end-user Task<TOut> instead of using ContinueWith - this will result in better error handling and fewer weird task context issues.
Replicates the same work we did on #7521
| var sub = await c.ExpectSubscriptionAsync(); | ||
| sub.Request(10); | ||
| c.ExpectError().Message.Should().Be("err2"); | ||
| (await c.ExpectErrorAsync()).Message.Should().Be("err2"); |
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.
We no longer get AggregateExceptions here unless the test completes immediately, in which case we might have to harden this in the future.
| var sub = await c.ExpectSubscriptionAsync(); | ||
| sub.Request(10); | ||
| c.ExpectError().InnerException.Message.Should().Be("err1"); | ||
| (await c.ExpectErrorAsync()).Message.Should().Be("err1"); |
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.
We no longer get AggregateExceptions here unless the test completes immediately, in which case we might have to harden this in the future.
Arkatufus
left a comment
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
Changes
Replicates some of the behaviors and fixes we made to
SelectAsyncin #7521Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):
SelectAsync#7521