Skip to content

Throw from OnConnected and OnDisconnected when using client results #42447

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 2 commits into from
Aug 5, 2022
Merged

Throw from OnConnected and OnDisconnected when using client results #42447

merged 2 commits into from
Aug 5, 2022

Conversation

BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy commented Jun 27, 2022 •
edited
Loading

Part of #5280

Don't allow Clients.Caller.InvokeAsync or Client.Client(...).InvokeAsync inside special hub methods OnConnectedAsync and OnDisconnectedAsync. These methods are meant to be used for connection management and are not made to work with blocking calls like ISingleClientProxy.InvokeAsync.

@BrennanConroy BrennanConroy added the area-signalr Includes: SignalR clients and servers label Jun 27, 2022
@BrennanConroy BrennanConroy requested a review from davidfowl June 27, 2022 16:21
@BrennanConroy BrennanConroy requested a review from halter73 as a code owner June 27, 2022 16:21
@BrennanConroy BrennanConroy requested review from davidfowl and halter73 and removed request for halter73 and davidfowl July 8, 2022 15:52
halter73
halter73 approved these changes Aug 4, 2022
View reviewed changes
Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

Annoying that it doesn't fix injected IHubContexts, but that's rare? Would invocations in OnDisconnected already fail?

@BrennanConroy BrennanConroy merged commit 27428b3 into main Aug 5, 2022
@BrennanConroy BrennanConroy deleted the brecon/noinvoke branch August 5, 2022 15:54
@ghost ghost added this to the 7.0-rc1 milestone Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@halter73 halter73 halter73 approved these changes

@davidfowl davidfowl Awaiting requested review from davidfowl

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

Successfully merging this pull request may close these issues.
2 participants
@BrennanConroy @halter73