Implement flow allowing disconnection from online services when another client instance for same user is detected #25522
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When the server requests a disconnect due to a user connecting via a second device, the client will now log the user out on the first device and show a notification informing them of the cause of disconnection.
2 tasks
peppy
reviewed
Nov 21, 2023
| @@ -98,7 +98,9 @@ public virtual void PartRoom() | |||
| if (JoinedRoom.Value == null) | |||
| return; | |||
|
|
|||
| api.Queue(new PartRoomRequest(joinedRoom.Value)); | |||
| if (api.State.Value == APIState.Online) | |||
There was a problem hiding this comment.
Without this check is it actually feasible that this fires and causes issues?
There was a problem hiding this comment.
Yes, I hit this once in testing.
That said I'll probably look at this again, after I'm somewhat sure this pair of PRs is going in the correct direction.
There was a problem hiding this comment.
Looking at this again, I think there should be no issues with this. Server-side flows are supposed to correctly clean up the room anyway, so this request not being realised should have no impact on the server.
peppy
reviewed
Nov 21, 2023
peppy
reviewed
Nov 21, 2023
|
I think this is looking pretty decent. |
peppy
approved these changes
Nov 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prerequisite for ppy/osu-server-spectator#80.
In action:
2023-11-21.15-12-55.mp4
In short, when a user logs in on one client instance, and then on another, the previous instance is forcibly logged out. It can be restored to full online operation by logging in again, at which point any other online instance will be forcibly logged out. So the goal here is to keep concurrency to 1.
The reason that this does a full logout rather than something more limited is (a) that it's easy to integrate with existing systems, and also (b) avoids weirdness. For instance, we could theoretically allow the user to solo even with concurrent client instances, were it not for the fact that
osu-server-spectator- namelySpectatorHub- participates in the replay recording part of score submission, and cannot handle concurrency.This currently has no testing other than manual, but I'm not sure anything more is feasible anyway. I'm also not sure of the general shape of this, which is why I'm opening as draft and refraining from adding tests until requested and I am sure that there's at least an agreement that this looks how everyone would want it to look.