Is it safe to range map without locking if multiple goroutines will run notifyAll func? Actually in a range I need to sometimes remove entries from a map.
var mu sync.RWMutex
func (self *Server) notifyAll(event *Event)
ch := make(chan int, 64)
num := 0
for k, v := range self.connections {
num++
ch <- num
go func(int k, conn *Conn) {
err := conn.sendMessage(event)
<-ch
if err != nil {
self.removeConn(k)
}
}(k, v)
}
}
func (self *Server) removeConn(int k) {
mu.Lock()
defer mu.Unlock()
delete(self.connections, k)
}
// Somewhere in another goroutine
func (self *Server) addConn(conn *Conn, int k) {
mu.Lock()
defer mu.Unlock()
self.connections[k] = conn
}
Or I must RLock map before range?
func (self *Server) notifyAll(event *Event)
mu.RLock()
defer mu.RUnlock()
// Skipped previous body...
}
You must lock the map to prevent concurrent reads in notifyAll
with the write in removeConn
.
Short answer: maps are not concurrent-safe (one can still say thread-safe) in Go.
So, if you need to access a map from different go-routines, you must employ some form of access orchestration, otherwise "uncontrolled map access can crash the program" (see this).
Edit:
This is another implementation (without considering housekeeping concerns - timeouts, quit, log, etc) which ignores the mutex all-together and uses a more Goish approach (this is just for demonstrating this approach which helps us to clear access orchestration concerns - might be right or not for your case):
type Server struct {
connections map[*Conn]struct{}
_removeConn, _addConn chan *Conn
_notifyAll chan *Event
}
func NewServer() *Server {
s := new(Server)
s.connections = make(map[*Conn]struct{})
s._addConn = make(chan *Conn)
s._removeConn = make(chan *Conn, 1)
s._notifyAll = make(chan *Event)
go s.agent()
return s
}
func (s *Server) agent() {
for {
select {
case c := <-s._addConn:
s.connections[c] = struct{}{}
case c := <-s._removeConn:
delete(s.connections, c)
case e := <-s._notifyAll:
for c := range s.connections {
closure := c
go func() {
err := closure.sendMessage(e)
if err != nil {
s._removeConn <- closure
}
}()
}
}
}
}
func (s *Server) removeConn(c *Conn) {
s._removeConn <- c
}
func (s *Server) addConn(c *Conn) {
s._addConn <- c
}
Edit:
I stand corrected; according to Damian Gryski maps are safe for concurrent reads. The reason that the map order changes on each iteration is "the random seed chosen for map iteration order, which is local to the goroutine iterating" (another tweet of him). This fact does not affect the first edit and suggested solution.