nex-go icon indicating copy to clipboard operation
nex-go copied to clipboard

Add MutexMap MutexSection/RMutexSection

Open PabloMK7 opened this issue 1 year ago • 7 comments

The previous implementation of the MutexMap didn't allow performing multiple operations (Get + Delete or Get + Set) in a mutually exclusive way, as locking the map manually with Lock() would cause a recursive lock when trying to call any of the methods. For that reason, I split the MutexMap into MutexMap and Map. Both structs have the same methods, but only the MutexMap methods actually lock the map. I also added MutexSection and RMutexSection methods to MutexMap, with runs a callback with a Map, that allows performing multiple operations to the map in an atomic way.

PabloMK7 avatar Apr 23 '24 08:04 PabloMK7

This change will also help refactoring https://github.com/PretendoNetwork/nex-protocols-common-go/pull/28

PabloMK7 avatar Apr 23 '24 08:04 PabloMK7

Some of the naming here would also likely be confusing/misleading. MapInterface would imply that it's an interface type, not a struct type

Also the use of the name Atomic here seems a bit misleading? The operations happening in the callback are not actually atomic, since all operations happen as they are called instead of all-or-nothing

mm := NewMutexMap[int, string]()

mm.Atomic(func(mapInterface *MapInterface[int, string]) {
	for i := 0; i < 10; i++ {
		fmt.Println(mapInterface.Size())

		if i == 5 {
			return
		}

		mapInterface.Set(i, "test")
	}
})

This prints size changes for every iteration. If this were truly atomic I would assume that no changes to mm would take place until after the callback finishes

jonbarrow avatar Apr 23 '24 15:04 jonbarrow

Some of the naming here would also likely be confusing/misleading. MapInterface would imply that it's an interface type, not a struct type

Also the use of the name Atomic here seems a bit misleading? The operations happening in the callback are not actually atomic, since all operations happen as they are called instead of all-or-nothing

mm := NewMutexMap[int, string]()

mm.Atomic(func(mapInterface *MapInterface[int, string]) {
	for i := 0; i < 10; i++ {
		fmt.Println(mapInterface.Size())

		if i == 5 {
			return
		}

		mapInterface.Set(i, "test")
	}
})

This prints size changes for every iteration. If this were truly atomic I would assume that no changes to mm would take place until after the callback finishes

It's atomic in concurrent terms. All the operations inside the callback will be done without other threads interfering. I'm open to name suggestions such as MutexSection. The best one I could find was Atomic

PabloMK7 avatar Apr 23 '24 16:04 PabloMK7

I'm failing to see why we need the MapInterface, can't we just give the real map as an argument on the Atomic callbacks?

To keep all the utility functions the same. That way if we find other places where this is needed we don't need to rewrite the logic too much.

PabloMK7 avatar Apr 23 '24 16:04 PabloMK7

It's atomic in concurrent terms. All the operations inside the callback will be done without other threads interfering

That's fair. I was thinking in terms of atomic operations typically seen in systems such as databases (but can be used outside of them) which take the form of "all-or-nothing" transactions. All operations either happen, or none do, with the results all applying at the same time if they do apply

These kinds of atomic operations here would be linearizability, so maybe taking some language from here would clear up the specific type of atomic operations being done here? Or at the very least updating the Godoc comment to note which kind of atomicity is being used

jonbarrow avatar Apr 23 '24 16:04 jonbarrow

Please re-review. The changes will be useful in the future even if the original bug this aimed to fix was fixed in a different way.

PabloMK7 avatar Jun 02 '24 10:06 PabloMK7