Implement thread safe interface for the OrderedMap

[nem0z]

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 

[elliotchance]

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]

@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?

[elliotchance]

@nem0z I think what you have already is good.

Yes, if the caller directly accesses the OrderedMap property (and any method on it) then they are intentionally circumventing the locking. That's not a bad thing though, for example, there might be a case where I hold a more general lock and perform a bunch of updates in one operation and so I don't need the individual method locks.

As for safe iterating, I think the best way is to provide a read-locked iterator and so all items can be processed or extracted safely. But this can be done in a separate PR, if you wish. Are you ready for me to accept this?

[nem0z]

Okay so what about exposing methods to take and release a read lock?
So basically all the methods implemented on SyncOrderedMap are tread-safe.
But you also have the ability to manage a read-lock yourself in case you want to interact with non-safe method?

For example with GetElement, you can safely get the element, and every time you use this element you can ensure it's not getting use elsewhere by using the read-lock?

I can update my PR to add these 2 methods and it should be good for me
@elliotchance

[nem0z]

@elliotchance I updated the PR with what we mentioned above + some extra tests

So basically it's possible to manually use the mutex so you can ensure operations are performed safely by yourself, which brings more flexibility.

This is tested with GetElement where I manually take a read-lock to access the value while other operation are happening at the same time.

I added an extra test which also serves as example for safe iteration. On thing to note and it's very important, if you manually take a read-lock, you cannot use sync methods before in the same goroutine before releasing the lock. This is because 1 single lock can be hold by each goroutine, this is an example:

	m.RLock()
	for key := range m.Keys() {
		m.Get(key)
	}
	m.RUnlock()

m.Get(key) would get blocked because this try to take a new read-lock while the goroutine already holds one.

So a solution would either be to use the non-sync method m.OrderedMap.Get(key), or make a call in a different goroutine:

	m.RLock()
	for key := range m.Keys() {
		go m.Get(key)
	}
	m.RUnlock()

And for this reason I don't know if it's really sensible to implement thread-safe version of the iterators, because this will abstract the lock taken by the iterator so users can be tempted to call a thread-safe method in the iterator scope and that would not work (the goroutine is blocked).

It's still possible to use the iterator by manually managing the lock as in the examples above.

If you're good with the current implemented we can go ahead and merge, then I can start working on the integration to v1 and v2.