Skip to content

Support IAsyncEnumerable returns in SignalR hubs #6791

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Feb 25, 2019
Merged

Support IAsyncEnumerable returns in SignalR hubs #6791

merged 7 commits into from
Feb 25, 2019

Conversation

halter73
Copy link
Member

I'm waiting for the dotnet SDK to get updated in this repo so I can target C# 8 in our test projects. I'll add tests once we have it.

#5357

@halter73 halter73 added the area-signalr Includes: SignalR clients and servers label Jan 17, 2019
@halter73 halter73 added the blocked The work on this issue is blocked due to some dependency label Feb 5, 2019
@halter73
Copy link
Member Author

halter73 commented Feb 5, 2019

Waiting on #7005

@halter73 halter73 removed the blocked The work on this issue is blocked due to some dependency label Feb 20, 2019
@halter73 halter73 force-pushed the halter73/5357 branch 4 times, most recently from 97c134d to 522a741 Compare February 22, 2019 02:41
halter73
halter73 commented Feb 22, 2019
View reviewed changes
src/SignalR/server/Core/src/Internal/AsyncEnumerableAdapters.cs
{
// Assume that this will be iterated through with await foreach which always passes a default token.
// Instead use the token from the ctor.
Debug.Assert(cancellationToken == default);
Copy link
Member Author

Choose a reason for hiding this comment

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

REVIEW: The reason we have to wrap IAsyncEnumerators with non-value-type generic parameters instead of just casting is because we iterate using await foreach which doesn't support flowing a CancellationToken through GetAsyncEnumerator. If we manually iterated over the enumerator, we could save ourselves wrapping in most cases.

So should we manually iterate instead of wrapping the GetAsyncEnumerator call?

@davidfowl

Copy link
Member Author

@halter73 halter73 Feb 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We are the only consumer of the IAsyncEnumerable<object> btw

Copy link
Member

Choose a reason for hiding this comment

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

So we could loop over MoveNextAsync() manually and avoid the BoxedAsyncEnumerator? That sounds nice :D

Copy link
Member

Choose a reason for hiding this comment

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

Yes we should optimize

Copy link
Member Author

Choose a reason for hiding this comment

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

You could have said something before I merged. It was a while loop that manually called GetAsyncEnumerator/MoveNextAsync until you asked me to change it to await foreach @davidfowl 😆

I figured didn't care about the allocation since a bunch of other allocations happen in this code path anyway. I created #7960 to track this with an XS tag. If we want to change this, it's not difficult.

Copy link
Member

Choose a reason for hiding this comment

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

I care, lets fix it in preview4. Assign it to yourself 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm glad you care 😉

@halter73 halter73 force-pushed the halter73/5357 branch 2 times, most recently from 0d00933 to 31c46d8 Compare February 23, 2019 00:14
@halter73
Support IAsyncEnumerable returns in SignalR hubs
4de77e9
@halter73
Bah, Humbug!
a42fff6 
I was hoping  typeof(T).IsValueType would get evaluated during JIT
compilation which would allow for dead code elimination, but alas:
https://github.com/dotnet/corefx/issues/16217
@halter73
Add tests for hubs returning IAsyncEnumerable
dbfadd1 
- Allow hub methods to also return types implementing IAsyncEnumerable
@halter73
Prefer IAsyncEnumerable over ChannelReader if hub method return type …
ec694c7 
…is both
@halter73
Use async foreach to consume IAsyncEnumerable
41544a2 
- Flow CancellationToken to IAsyncEnumerable.GetAsyncEnumerator
@halter73
Use IAsyncEnumerable returned by ChannelReader.ReadAllAsync()
646060a
@halter73 halter73 merged commit 46fe595 into master Feb 25, 2019
@halter73 halter73 deleted the halter73/5357 branch February 25, 2019 23:08
@halter73 halter73 mentioned this pull request Feb 26, 2019
Closed
2 tasks
@bradygaster bradygaster mentioned this pull request Feb 26, 2019
Closed
16 tasks
@halter73 halter73 mentioned this pull request Feb 26, 2019
Closed
@benaadams benaadams mentioned this pull request Mar 3, 2019
Merged
@mikaelm12 mikaelm12 mentioned this pull request May 9, 2019
Merged
@ladeak ladeak mentioned this pull request Apr 23, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidfowl davidfowl davidfowl left review comments

@JamesNK JamesNK JamesNK left review comments

@campersau campersau campersau left review comments

@BrennanConroy BrennanConroy BrennanConroy approved these changes

@mikaelm12 mikaelm12 Awaiting requested review from mikaelm12

Assignees
No one assigned
Labels
area-signalr Includes: SignalR clients and servers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@halter73 @davidfowl @JamesNK @campersau @BrennanConroy