Skip to content

Fix AddToGroupAsync race with OnDisconnectedAsync #42117

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 1 commit into from
Jun 10, 2022
Merged

Fix AddToGroupAsync race with OnDisconnectedAsync #42117

merged 1 commit into from
Jun 10, 2022

Conversation

BrennanConroy
Copy link
Member

Fixes #38531

Also snuck in a capture removal while I was messing with that area of the code.

@BrennanConroy BrennanConroy added the area-signalr Includes: SignalR clients and servers label Jun 9, 2022
@BrennanConroy BrennanConroy requested a review from davidfowl June 9, 2022 17:37
@BrennanConroy BrennanConroy requested a review from halter73 as a code owner June 9, 2022 17:37
halter73
halter73 approved these changes Jun 10, 2022
View reviewed changes
Copy link
Member

@halter73 halter73 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This has no functional impact other than to use less memory and keep around less subscriptions, right?

We could have HubConnectionStore use WeakReference<HubConnectionContext> and clean up periodically. Then it wouldn't matter if we missed an unsubscribe! 😉

@davidfowl
Copy link
Member

We could have HubConnectionStore to keep WeakReference and clean up periodically. Then it wouldn't better if we missed an unsubscribe! 😉

No thanks 😄

@davidfowl
Copy link
Member

Or subscribe to the token.

@BrennanConroy BrennanConroy merged commit c7c3fc7 into main Jun 10, 2022
@BrennanConroy BrennanConroy deleted the brecon/group branch June 10, 2022 20:58
@ghost ghost added this to the 7.0-preview6 milestone Jun 10, 2022
captainsafia pushed a commit to captainsafia/aspnetcore that referenced this pull request Jun 13, 2022
@BrennanConroy @captainsafia
Fix AddToGroupAsync race with OnDisconnectedAsync (dotnet#42117)
21495ba
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 davidfowl approved these changes

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

Successfully merging this pull request may close these issues.

Signalr AddToGroupAsync possible race condition
3 participants
@BrennanConroy @davidfowl @halter73