-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Forcefully leave room on multiplayer exit #35971
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
Conversation
| if (Room == null) | ||
| return Task.CompletedTask; |
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.
I don't really know why this was added or necessary, because it doesn't look to be. Basically, there's a moment in time (that can actually be quite lengthy due to user lookups), where the room is "joined" but Room hasn't been set. The patch in the OP shows this.
It seems like we should always run through the full leave process?
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.
Originates from 02369ba. Appears it wasn't asked about at review time, probably because it looked obvious / harmless.
bdach
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.
seems probably fine
| if (Room == null) | ||
| return Task.CompletedTask; |
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.
Originates from 02369ba. Appears it wasn't asked about at review time, probably because it looked obvious / harmless.
| if (base.OnExiting(e)) | ||
| return true; | ||
|
|
||
| client.LeaveRoom().FireAndForget(); |
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.
While we're here doing this, the xmldoc says this can throw:
| /// <exception cref="NotJoinedRoomException">If the user is not in a room.</exception> |
but looking at the server-side implementation I don't see how it can throw because this specific scenario is guarded to do nothing instead of throwing, so might as well strip that mention.
Fixes #35886 (comment)
Seems pretty difficult to unit test this so I've only tested via:
Will document more in comments on the PR itself.