Refactor of matchmaking_utils.go

[PabloMK7]

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.

[jonbarrow]

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]

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

[PabloMK7]

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]

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

[jonbarrow]

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?

[PabloMK7]

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.

[jonbarrow]

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)

[PabloMK7]

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.

[PabloMK7]

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]

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

[PabloMK7]

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