Is there a way to syncronise map reads and writes?

[Zhaoxun Yan]

Hi guys!

I know this is quite challenging, but it makes sense in io-heavy applications.

I need to test the responsiveness of different financial future quotation sources. They return the quotations spontaneously, and my program respond to each one of the CGO call backs from the quotation library and save the current prices. Meanwhile, a timer ensures to check the current quotations of each available sources. And obviously, it raised up a panic due to synchronization of maps on the call of ReadMaps function. Unfortunately this came up occasionally and `go build -race` cannot raise the problem.

Here is a piece of the code, only the read/write part:

------------------------------------------------------------------------------------------

// Please Ignore SyChan to alert the timer that all available sources are collected

var SyChan chan int = make(chan int, 3)

// Make up a map of 5 mutex locks and a map of 5 data structs, indexed by 1-5

var marketMaps = make(map[int]map[string]Market)
var mapLocks = make(map[int]*sync.Mutex)

// quotation data
type Market struct {
        Price     float64 `json:"price"`
        Timestamp string  `json:"timestamp"`
}

func init() {
        for i := 1; i <= 5; i++ {
                mapLocks[i] = new(sync.Mutex)
                marketMaps[i] = make(map[string]Market)
        }
}

//Make sure that for each source, R/W has no race, only took place as acquiring the particular Mutex inside the map of Mutex.
func RWMaps(targetnum int, purpose, future_id string, market Market) map[string]Market {
   
        mapLocks[targetnum].Lock()
        defer mapLocks[targetnum].Unlock()

        if purpose == "update" {//The original Write part
                marketMaps[targetnum][future_id] = market
                   return nil
        } else { //The read part, has been extracted to ReadMaps
                SyChan <- 1
                return marketMaps[targetnum]
        }
}

//Here is why I use map: not all 5 sources must be available, some may have connection failure and would be marked as false in Usable[i] , i being its source No.

func ReadMaps(targetnum, checkTime int) map[string]Market {
        mapLocks[targetnum].Lock()
        defer mapLocks[targetnum].Unlock()
        if Usable[targetnum] {
                fmt.Printf("%d-th time to read source %d \n", checkTime, targetnum)
        }

        SyChan <- 1
        return marketMaps[targetnum]
}

--------------------------------------------------------------------------------------------------

My problem is :

I still want to keep the map structure, rather than naming mutex1, mutex2, mutex3,... , marketmsg1, marketmsg2, .... And obviously if the map prohibits reading or writing spontaneously, the writing by each usable sources is not fair - They must line up as one queue (like using one Mutex for all instead) hence the checking of quotation snap on a time-spot is defected.  I have also checked the sync.Map and it seems to allow spontaneous reading but still prohibits spontaneous writing.

My instinct is to make map on the pointer of structs. On that way, if I could use sync.Map, both the read and write function only read the map to find the address of data structs, then writing several data structs on the same time won't violate the rule on sync.Map.

Is the proposal above legitimate? And anyone could come up a test case to mimic the quotation sources? Because with CGO to complicate the program, go cannot raise the race problem on -race check, and the successful build can panic occasionally.

Thanks in advance!

[robert engels]

Something else is wrong - because marketMaps is read-only after init… so unless you have some other code - that is not the map causing the panic.

My guess is because you are returning the map from ReadMaps (or RWMaps) that you are using THAT map by multiple threads outside the lock (the map is a reference not a copy) and that is causing the panic.

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/9a981200-23f8-4dae-8c20-7acfdcd3f2fcn%40googlegroups.com.

[Jason E. Aten]

You have not synchronized access to the top level mapLocks map itself.  Thus you will get races. You need to have a mutex that you lock before you access mapLocks; not just the maps that are inside it.

I would also recommend being a little object oriented about this design, putting each map inside its own struct, and make a set of accessor methods that Lock() and defer Unlock() on a mutex that protects the maps inside.  Read code like this for an idea of what I mean. https://github.com/glycerine/atomicmap/blob/master/amap.go

[Robert Engels]

The top level map locks doesn’t matter - it is write once at init and read only after. 

On Feb 22, 2022, at 11:51 PM, Jason E. Aten <j.e....@gmail.com> wrote:

You have not synchronized access to the top level mapLocks map itself.  Thus you will get races. You need to have a mutex that you lock before you access mapLocks; not just the maps that are inside it.

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/215a541a-ecc6-49e2-9427-81f62c5b168en%40googlegroups.com.

[Jason E. Aten]

Unfortunately, that's not how Go maps work. You'll still get races unless you synchronize even "read-only" maps.

[Brian Candler]

On Wednesday, 23 February 2022 at 03:39:45 UTC yan.z...@gmail.com wrote:

I have also checked the sync.Map and it seems to allow spontaneous reading but still prohibits spontaneous writing.

I'm not sure what you mean by that.  Do you really mean "spontaneous" or "simultaneous"? sync.Map serializes reads and writes to the map.  If two goroutines try to update the map simultaneously, the updates will happen safely, one after the other.  It doesn't prohibit spontaneous writes, only ensures that one is delayed until the other is complete.

More importantly, sync.Map doesn't synchronize anything else. So if map["foo"] contains a pointer to some struct Bar, and two goroutines both try to access the same Bar, they will conflict with each other.

It's a fundamental issue here that a consumer could request the current quotation for stock X at the same instant as a new update can come in for stock X asynchronously. You are going to have to serialize this one way or another so that the consumer reading the current quotation is guaranteed to see it how it was either before *or* after the update, not during.

I'd say you need to rethink your concurrency approach.  My first reaction is to have a separate goroutine which maintains all the current quotations, and has a channel which accepts messages saying "update quotation" or "read quotation".  The "read quotation" message can include a response channel as one of its members.

If you still feel you want to go down the mutex locking approach, then at least first watch and digest this video, "Rethinking Classical Concurrency Patterns" by Bryan C Mills:

https://www.youtube.com/watch?v=5zXAHh5tJqQ

[Tamás Gulácsi]

Use a slice instead of a map ?

[Zhaoxun Yan]

Hi guys!

Thanks for the reply. The scenario is the program booked live financial quotation feeds by a dynamic library (.so in Linux and .dll in Windows) from up to 5 different live data sources. It is like testing which CDN provides faster feeds or faster live streaming videos. It is like booking several live football matches from 5 different online stations and check from time to time to compare each match at different data sources.

If I check 10 times that source 1 always has the latest scene of all matches, I judge it as the best one.

My goal is for the different data sources to write in current messages simultaneously -  that source 1 writing on match 1 will queue with the source 1 writing on match 3, and queue with reading all current match messages by source 1 if needed, BUT not queue with  source2 writing on match 1.

 The conservative way to do it is to declare data1,... data5 and lock1,...,lock5. But as the code shows, I chose to organize the 5 data structs and 5 mutex locks into two maps. It saves codes indeed and allows for scaling up, e.g. 20 data sources. But it caused panic - golang map is unsafe among goroutines - if a map is being read or written one key-value, it cannot accept a call to read or write even another key-value otherwise a panic would occur.

I have heard that sync.Map allows for reading simultaneously among multiple goroutines. My proposal is to substitute the map of Market data to a map of *Market data. In that way the map only stores the where the real data is in memory, which would keep unchanged after initialization and will not cause trouble even if two cores are changing the real data of source 1 and of source 2 at the same time.

Is it valid? How to test it without the dynamic libraries?

[Brian Candler]

On Wednesday, 23 February 2022 at 08:49:26 UTC yan.z...@gmail.com wrote:

I have heard that sync.Map allows for reading simultaneously among multiple goroutines.

It does.  But it doesn't synchronize what you do with the values returned from the map.  So if you retrieve a *copy* of a value, or replace the value, it's fine.  But if you retrieve a pointer, and then either read via that pointer or update the object via that pointer, you'll be back to a race condition.  You should note that various Go data types include pointers internally, especially slices.  However in your case, you're proposing that a sync.Map (safe) contains pointers to *Market data (unsafe to access concurrently).  So you would still need a mutex to protect each Market object, and every read and write would have to apply that mutex.

Personally, I would avoid locks and use channels instead.  It's much easier to reason about.  Have one goroutine per market, and have it accept 'update' and 'read' messages over a channel (or two different channels).

[Zhaoxun Yan]

Hi Brian, thanks for replying!

So you mean that I have to use 5 Mutexes for Market data of each one source ? That is exactly what I want to do. Every action of RW marketMap[i] must comes in one line, but RW marketMap[j] concurrently is allowed. And mapLocks now is fine in golang?

The reason why I do not use channel is that not each one market updates is needed, but I need to take samples from time to time.

--
You received this message because you are subscribed to a topic in the Google Groups "golang-nuts" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-nuts/6I_kS6Pya0M/unsubscribe.
To unsubscribe from this group and all its topics, send an email to golang-nuts...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/1f5beaae-e5d0-4e13-bbe1-28d28ee47e9dn%40googlegroups.com.

[Brian Candler]

If you have a static map of mutexes, and a map of *Market, then you don't need sync.Map - because all of the accesses to the maps are read-only. If you are inserting new data sources while the program is running, then you *do* want sync.Map.

On Wednesday, 23 February 2022 at 10:43:27 UTC yan.z...@gmail.com wrote:

The reason why I do not use channel is that not each one market updates is needed, but I need to take samples from time to time.

Yes, that's quite possible with a goroutine that manages the market state:

https://go.dev/play/p/4Q8H-Binqc1

You can have as multiple writers and multiple readers.  As long as communication is over a channel, and you copy data over the channel rather than copying pointers (or implied pointers like slices), then it's all safe and race-free.

Another way to enforce the same discipline is if you keep your markets and mutexes private, and enforce all access via getter/setter methods.

[robert engels]

That is not correct.

See https://go.dev/play/p/aZ7LostRuKR 

You can run this under the race detector without failure.

Still, the race detector cannot detect all races so better to look at the source for sync.Map at 105

You can see that it reads the underlying shared maps while not under lock/exclusive access.

Starting a go routine creates a “happens before” relationship which allows this to work.

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/0b709930-8fbb-4fce-8cdf-0cf3bb401d22n%40googlegroups.com.

[Zhaoxun Yan]

Hi Brian!

Your code with a map of channels is quite impressive! How can I test if the code has no races in case I have many go files inside a folder?

`go build -race`?

package main

import "fmt"

type Market struct {
Price     float64 `json:"price"`
Timestamp string  `json:"timestamp"`
}

type Command int

const (
SetPrice Command = iota
ReadLatest
)

type CmdSetPrice struct {
Price float64
}

type CmdReadLatest struct {
Response chan RespReadLatest
}

type RespReadLatest struct {
Price float64
}

func MarketManager(c chan interface{}) {
var current Market // hidden internal state
for msg := range c {
switch data := msg.(type) {
case CmdSetPrice:
current.Price = data.Price
//current.Timestamp = ...
case CmdReadLatest:
data.Response <- RespReadLatest{Price: current.Price} // returns copy of data, not pointer
}
}
}

func RunMarketManager() chan interface{} {
c := make(chan interface{})
go MarketManager(c)
return c
}

func main() {
markets := make(map[string]chan interface{}) // use sync.Map if you add/remove markets while it's running
markets["Dow"] = RunMarketManager()
markets["Nikkei"] = RunMarketManager()
fmt.Println("Market is open")

// Now all reads and writes are via channels, whenever you like
res := make(chan RespReadLatest)

markets["Dow"] <- CmdSetPrice{Price: 123.0}
markets["Dow"] <- CmdSetPrice{Price: 125.0}
markets["Dow"] <- CmdReadLatest{Response: res}
fmt.Println(<-res)

markets["Dow"] <- CmdSetPrice{Price: 126.0}
markets["Dow"] <- CmdSetPrice{Price: 122.0}
markets["Dow"] <- CmdReadLatest{Response: res}
fmt.Println(<-res)

//Clean shutdown of goroutines
close(markets["Dow"])
close(markets["Nikkei"])
}

You received this message because you are subscribed to a topic in the Google Groups "golang-nuts" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-nuts/6I_kS6Pya0M/unsubscribe.
To unsubscribe from this group and all its topics, send an email to golang-nuts...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/A04B8C4C-3C0C-4077-9FCF-5632D359C664%40ix.netcom.com.

[Jason E. Aten]

I stand corrected. I had been recalling programs with READ/READ races but those must have writes interspersed too and not just reads. I am also surprised that the designers would give up the freedom to change the caching strategy under the hood. But it appears read-only maps are guaranteed to work, since it is there in the language documentation FAQ:

"As long as all goroutines are only reading—looking up elements in the map, including iterating through it using a for range loop—and not changing the map by assigning to elements or doing deletions, it is safe for them to access the map concurrently without synchronization." -- from https://go.dev/doc/faq#atomic_maps

[Brian Candler]

On Wednesday, 23 February 2022 at 13:51:23 UTC yan.z...@gmail.com wrote:

Hi Brian!

Your code with a map of channels is quite impressive! How can I test if the code has no races in case I have many go files inside a folder?

`go build -race`?

I believe so (or "go build -race ." with the dot)

Now, I was thinking to myself that the race detector isn't necessary here.  Since there are no global variables and no pointers being passed over channels, it's provably race-free.

But there is one wrinkle, which is that interface values are implicitly boxed pointers. Roughly speaking, the box contains a <type, pointer> pair.

Now, I believe that interface values are copied before being boxed/unboxed, and I believe that it's not possible to use interfaces for aliasing, in a way that mutating one copy of the interface value affects another one. But I don't know where exactly it says so in the language spec.  At least, I've not found a way to make this aliasing happen:

https://go.dev/play/p/GAXybxUxfqJ

If that's guaranteed by the language, then the code is good as it stands, but it remains a subtle point.  The only reason I used an interface in the first place was as a way of carrying different structs over the same channel, effectively using it as a tagged union.