Skip to content
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

Refactor of matchmaking_utils.go #28

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

PabloMK7
Copy link
Contributor

@PabloMK7 PabloMK7 commented Apr 10, 2024

This change implements the following things:

  • Add a SessionManagementDebugLog boolean to allow debugging general session state.
  • Made the sessions map private and add a lock, to prevent race conditions.
  • Changed the behaviour of GatheringFlags.DisconnectChangeOwner flag. Only the owner is changed and the host is left untouched.
    • Fixes gatherings getting stuck.
  • Changed the behaviour of UpdateSessionURL and UpdateSessionHost. Only the host is changed and the owner is left untouched.
  • Send the GatheringUnregistered event when a gathering is deleted.

I will implement this change in the CTGP-7 server to investigate the effects. If they are positive will convert from a draft to a PR.

@DaniElectra DaniElectra requested review from DaniElectra and jonbarrow and removed request for DaniElectra April 10, 2024 20:43
@jonbarrow
Copy link
Member

Is there a reason you didn't use MutexMap from nex-go here? We already provide a data structure for this kind of functionality

@PabloMK7
Copy link
Contributor Author

PabloMK7 commented Apr 10, 2024

Because when I started implementing it I didn't know what I wanted yet. If stuff works it can be migrated.

@PabloMK7
Copy link
Contributor Author

PabloMK7 commented Apr 11, 2024

Reviewed MutexMap and it doesn't really provide what's needed. It only fixes concurrent accesses to the map, but doesn't allow locking it for entire functions, which is what's needed here.

EDIT: It's actually possible to lock the mutexmap manually by calling Lock or Unlock on it, but then it's not possible to access its members without locking it again, causing deadlocks.

@PabloMK7
Copy link
Contributor Author

PabloMK7 commented Apr 11, 2024

Ready for review after leaving the server running overnight and not noticing any issues nor stuck rooms in MK7.

@PabloMK7 PabloMK7 marked this pull request as ready for review April 11, 2024 08:48
globals/matchmaking_utils.go Show resolved Hide resolved
globals/matchmaking_utils.go Outdated Show resolved Hide resolved
globals/matchmaking_utils.go Outdated Show resolved Hide resolved
match-making/update_session_host.go Outdated Show resolved Hide resolved
globals/matchmaking_utils.go Outdated Show resolved Hide resolved
globals/matchmaking_utils.go Outdated Show resolved Hide resolved
match-making/update_session_url.go Outdated Show resolved Hide resolved
globals/matchmaking_utils.go Outdated Show resolved Hide resolved
// FindOtherConnectionID searches a connection ID on the session that isn't the given one
// Returns 0 if no connection ID could be found
func FindOtherConnectionID(excludedConnectionID uint32, gatheringID uint32) uint32 {
func findOtherConnectionIDImpl(excludedConnectionID uint32, gatheringID uint32) uint32 {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a huge fan of the way these Impl functions are named/handled, though I understand why the implementation is separate from the exported function

This Impl convention could be refactored out by moving all of this out of the global scope and moving it to a struct, and having the functions currently named with Impl as just unexported methods

Something like

package main

import "fmt"

type sessionsManager struct{}

func (sm *sessionsManager) FindOtherConnectionID() {
	fmt.Println("Exported")

	sm.findOtherConnectionID()
}

func (sm *sessionsManager) findOtherConnectionID() {
	fmt.Println("Unexported")
}

func main() {
	sm := &sessionsManager{}

	sm.FindOtherConnectionID()
	// Prints both Exported and Unexported
}

@DaniElectra thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Waiting for DaniElectra response, it's also possible to not export the functions by just making them lowercase. But it may be better to have the sessions maps in the struct instead of being something global.

Copy link
Member

Choose a reason for hiding this comment

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

it's also possible to not export the functions by just making them lowercase

That's already how it's implemented, both in your PR and my suggestion? My comment was about exported vs unexported, it was on the naming conventions of the methods

It's clear you want to export a function which calls an implementation, without exposing the implementation itself. This is fine, I'm just not a fan of the way the functions themselves are named currently. So my suggestion was to bring them into a "manager" style struct, renaming the Impl functions to just have the same name as the exported one but be unexported

Though now that I think about it, why is the implementation functions separate now? I see elsewhere you call the Impl functions directly, but since both the exported and Impl functions have the same signatures why not just call the exported function directly and not have this separation?

But it may be better to have the sessions maps in the struct instead of being something global.

My example was minimal to try and get the point across but yes ideally in this example everything to do with sessions would be contained in the manager (the lock, the map, etc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The impl is needed to be able to call functions without locking the sessions lock again. I will move everything to a struct after I figure out the stuck gathering issues.

globals/matchmaking_utils.go Outdated Show resolved Hide resolved
globals/matchmaking_utils.go Outdated Show resolved Hide resolved
globals/matchmaking_utils.go Outdated Show resolved Hide resolved
matchmake-extension/get_simple_playing_session.go Outdated Show resolved Hide resolved
@PabloMK7
Copy link
Contributor Author

I just added the HostChanged notification to UpdateSessionURL, but I made it only send the notification to the old gathering host, and below is why.
Looking at the user dumps, I have made the following observations:

  • Observation 1: After calling UpdateSessionURL an OwnerChanged notification SOMETIMES is being sent, and the new owner PID doesn't have to match the user who called the method.
  • Observation 2: When an user receives a HostChanged notification, they ALWAYS call EndParticipation afterwards.

Here is what I think is happening:

  1. At some point, the room host becomes unstable enough for the rest of the clients to agree on calling UpdateSessionURL. The server updates the session host and sends the HostChanged notification to the old host only.
  2. The old host receives the HostChanged notification and leaves the gathering, calling EndParticipation.
  3. The server receives the EndParticipation, and sends the OwnerChanged notification to the rest of the players if they were the owner.

This theory explains both observations:

  • Observation 1: In the cases when host == owner, we see the OwnerChanged notification because the host was kicked and was the owner. In the cases when host != owner, we don't see any notifications because the owner didn't change and the HostChanged notification is not send to everyone.
  • Observation 2: The HostChanged notification causes the host to leave the room, that's why it always comes just before EndParticipation.

@jonbarrow
Copy link
Member

That all sounds logical to me. Barring any new observations or issues with that implementation I think that's fine?

@PabloMK7
Copy link
Contributor Author

From my observations the NewParticipant notification is only sent to the owner in MK7. This has to be verified with other games (cc @shutterbug2000 )

@DaniElectra DaniElectra mentioned this pull request Jun 28, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants