Skip to content

Conversation

nem0z
Copy link

@nem0z nem0z commented Sep 19, 2025

This PR add a struct SyncOrderedMap which is basically a Thread safe wrapped to OrderedMap
As mentioned in this comment: #62 (comment)

I understand implementing thread safe operation directly in the OrderedMap isn't what we want as it would add an extra complexity to scenarios where thread safe isn't required, adding another struct SyncOrderedMap give the consumer the choice to use or not the thread safe feature.

I assumed that methods such as GetElement and the iterators are not really compatible with thread safe given that it will provide access to the inner component of the OrderedMap which could then be used a non thread safe manner.
Example we could totally return a *Element safely but using this Element to read/write the key or the value would kind of "corrupt" the SyncOrderedMap

For the same reason I didn't support NewOrderedMapWithElements for SyncOrderedMap, however I think a method on OrderedMap or another constructor for SyncOrderedMap which would create a new SyncOrderedMap from an existing OrderedMap could be interesting, this would need to copy the value though, as we don't want values to be still accessible from a non thread safe OrderedMap.

Test:
I added a unit test verifying that the SyncOrderedMap is race condition free, not sure if it's enough for you?
I think we could reimplement some of the test for this new struct to make sure it doesn't introduce any regression?

Question:
Should I also put my changes on v1 and v2 given that it doesn't introduce any breaking chance?

@elliotchance


This change is Reviewable

@elliotchance
Copy link
Owner

Thanks for this! I'm happy to accept this, with one small change:

I assumed that methods such as GetElement and the iterators are not really compatible with thread safe given that it will provide access to the inner component of the OrderedMap which could then be used a non thread safe manner.
Example we could totally return a *Element safely but using this Element to read/write the key or the value would kind of "corrupt" the SyncOrderedMap

I think it's reasonable to note in the docs for GetElement that the value returned is only safe to read but any mutations needs synced separately.

Question:
Should I also put my changes on v1 and v2 given that it doesn't introduce any breaking chance?

Please create separate diffs v1 and v2 as they will be released separately as new minor versions.

@nem0z
Copy link
Author

nem0z commented Sep 19, 2025

@elliotchance thanks for the quick feedback
I'm actually not sure GetElement will return a value which is safe to read, for example if you try to print element.Value it might cause race condition if another process is writing to this element.

I tried to add this test to TestRaceCondition:

	var asyncGetElement = func() {
		wg.Add(1)
		go func() {
			key := rand.Intn(100)
			e := m.GetElement(key)
			if e != nil {
				fmt.Println(e.Value)
			}
			wg.Done()
		}()
	}

Running the test using go test -race I got:

--- FAIL: TestRaceCondition (0.28s)
    testing.go:1617: race detected during execution of test
FAIL
exit status 1
FAIL	github.com/elliotchance/orderedmap/v3	0.585s

We can think about some scenario where the GetElement is called in a concurrent access context but the element is eventually access (read/write) in a non-concurrent context. Not sure if it justify supporting in the Thread safe struct?

Please create separate diffs v1 and v2 as they will be released separately as new minor versions.

You mean a separate PR for v1 and v2 is it?

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.

2 participants