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

Add MutexMap MutexSection/RMutexSection #47

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

PabloMK7
Copy link
Contributor

@PabloMK7 PabloMK7 commented Apr 23, 2024

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
Copy link
Contributor Author

This change will also help refactoring PretendoNetwork/nex-protocols-common-go#28

Copy link
Member

@DaniElectra DaniElectra left a comment

Choose a reason for hiding this comment

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

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?

@jonbarrow
Copy link
Member

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

@PabloMK7
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@jonbarrow
Copy link
Member

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

@PabloMK7 PabloMK7 force-pushed the slidingwindowfix branch from 4cb78b1 to 9da218d Compare June 2, 2024 10:03
@PabloMK7 PabloMK7 changed the title Add MutexMap Atomic/RAtomic and fix sliding window crash Add MutexMap MutexSection/RMutexSection Jun 2, 2024
@PabloMK7 PabloMK7 requested a review from DaniElectra June 2, 2024 10:05
@PabloMK7
Copy link
Contributor Author

PabloMK7 commented Jun 2, 2024

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.

mutex_map.go Outdated Show resolved Hide resolved
@PabloMK7 PabloMK7 force-pushed the slidingwindowfix branch from 9da218d to 008e0d2 Compare June 2, 2024 11:44
Copy link
Member

@DaniElectra DaniElectra left a comment

Choose a reason for hiding this comment

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

Just a little typo

mutex_map.go Outdated Show resolved Hide resolved
Comment on lines +5 to +8
// RawMap implements a map type with helper functions to operate it.
type RawMap[K comparable, V any] struct {
real map[K]V
}
Copy link
Member

Choose a reason for hiding this comment

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

This can just be made into a type alias rather than creating a whole struct

Suggested change
// RawMap implements a map type with helper functions to operate it.
type RawMap[K comparable, V any] struct {
real map[K]V
}
// RawMap implements a map type with helper functions to operate it.
type RawMap[K comparable, V any] map[K]V

Then it can just be used like a regular map, how real is being used internally. Though I'm not sure what the benefit of this type is right now, since all it seems to do is wrap a map and then give it helper functions for things Go already supports?

Like why use RawMap.Delete() rather than just using a normal map and the built in delete()?

Comment on lines 10 to 14
// MutexMap implements a map type with go routine safe accessors through mutex locks. Embeds sync.RWMutex
type MutexMap[K comparable, V any] struct {
*sync.RWMutex
real map[K]V
mutex *sync.RWMutex
rawMap RawMap[K, V]
}
Copy link
Member

Choose a reason for hiding this comment

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

We prefer if structs are defined directly before their methods, so this should be moved further down

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