Concurrency fixes

[brian-nguyen]

Well-known data races exist, but strangely, none of them has led to any catastrophic behaviour (as far as we can tell). We should be proactive and fix all possible data races before one of them breaks something. A good start would be to update our slice/map to be concurrency-safe, then considering what functions need additional synchronization.

[SeeratSek]

add mutex to holes, players, junk maps

[bobheadxi]

I had a poke into this this morning 😋

add mutex to holes, players, junk maps

I don't think this is enough - while this is helpful for protecting the maps and the pointers they contain, it doesn't protect against modifications to individual structs effectively, because modifying just one struct will block access to the entire map. I think you can use RLock() to retrieve items from maps, then when doing calculations based on individual structs you'll want to chuck a mutex on each struct, make all values private, and protect things on a struct-by-struct basis, such as:

// Hole contains the data for a hole's position and size
type Hole struct {
        // ... everything private
	m             sync.RWMutex
}

// Update reduces this holes life and increases radius if max not reached
func (h *Hole) Update() {
	h.M.Lock()
	h.life--

	if h.Life < h.StartingLife-HoleInfancy {
		h.isAlive = true
	}
	if h.Radius < MaxHoleRadius*1.2 {
		h.radius += 0.02
		h.gravityRadius += 0.03
	}
	h.m.Unlock()
}

this means stuff in package arena should not be attempting to access values directly, otherwise you'll have to leave Lock() calls everywhere to protect each struct. Seems tricky tho so I dunno 🤷‍♂️

[SeeratSek]

I realized this earlier today too! I'm going to try creating my own concurrentMap type which will contain the original map of key -> value as well as an additional map of key -> lock so that each struct has it's own ReadWrite lock. Obviously this will increase the space used but I think that's doable. Let me know if you see any issues with this approach! @bobheadxi

[bobheadxi]

I think that might work 👍

[brian-nguyen]

so that each struct has it's own ReadWrite lock

Instead of another map, can a RWMutex be added to each of our types (structs)? We already had that idea in place when a mutex was added to our Player type. Can you elaborate on the 2nd map approach?

If you wanna build your own map type, check out sync.Map too. It seems promising. I don't think there isn't an equivalent for slices tho. :(

[bobheadxi]

can a RWMutex be added to each of our types (structs)?

This is the approach I suggested, but either way works I think, so long as access is enforced through the custom map. This way you could have functionality similar to sync.Map while supporting slices to some degree

[brian-nguyen]

This is the approach I suggested, but either way works I think, so long as access is enforced through the custom map. This way you could have functionality similar to sync.Map while supporting slices to some degree

Riiight. Thank you for your input :)

@SeeratSek if your custom type can support both maps and slices, that'd be reaaaally dope

[SeeratSek]

update: for now, we will be creating a concurrentmap type as well as a concurrentslice type. Hopefully will be able to eventually support both at once