Fix deadlock in PacketResendManager

[PabloMK7]

Without this change, PacketResendManager.Clear() was deadlocking.

[PabloMK7]

Updated the title, I thought RLock/RUnlock meant Recursive Lock/Recursive Unlock, which isn't the case. The issue is now properly fixed by implementing a Clear method with a callback in MutexMap

[DaniElectra]

Good catch! The issue is probably happening because the RLock from MutexMap.Each is not released while ranging through the map values, which causes the PacketResendManager.Remove function to wait forever to Lock the map access, effectively causing the deadlock.

A more simple way to fix the issue would be to RUnlock the access at the start of the ranging, and RLock it again at the end. See as a reference: https://github.com/PretendoNetwork/nex-go/blob/7f23e056c1e3262a14026aa2f0150c75ed76f356/server.go#L903-L919

On MutexMap.Each, this would look like this:

// Each runs a function for every item in the map
// the function takes in the items key and value
func (m *MutexMap[K, V]) Each(callback func(key K, value V)) {
    // * https://stackoverflow.com/a/40456170
    // TODO - MAKE A BETTER API FOR RANGING OVER THIS DATA!
    m.RLock()
    defer m.RUnlock()
    for key, value := range m.real {
        m.RUnlock()
        callback(key, value)
        m.RLock()
    }
}

[PabloMK7]

I also thought of that, but I didn't do it because I wanted to keep all the MutexMap methods atomic. That way, the Each method wouldn't be atomic and the map could be modified while iterating (I doubt it would cause any issues, but it breaks the concept of MutexMap).

[jonbarrow]

That makes sense. Maybe we should also add a note on MutexMap.Each about not allowing modifications of the map?

Yes, a note should be made. That's my fault. I originally made MutexMap.Each with the intention of not allowing modifications (though I was open to the idea of adding them), as I agree with Pablo. It breaks the idea of a "safe map"

You can see this somewhat noted here inside handleAcknowledgement, though that's far from proper documentation https://github.com/PretendoNetwork/nex-go/blob/7f23e056c1e3262a14026aa2f0150c75ed76f356/server.go#L277-L288

Maybe in the future something like a UnsafeEach method could be added which unlocks and allows for modifications? But I don't think that's necessary right now

[PabloMK7]

Maybe in the future something like a UnsafeEach method could be added which unlocks and allows for modifications? But I don't think that's necessary right now

The only real case this could be useful would be to remove some of the entries of the map. For that, a Filter function could be added with a callback that returns true if the item should be actually removed. But that's not needed for now.

[jonbarrow]

Lgtm