CODEWITHSUNDEEP
dictionarygoconcurrency
leesper
leesper

Reputation: 21

What's wrong with this concurrent map?

Recently I have developed a golang TCP network programming framework name Tao, in file util.go there's a concurrent map called ConnectionMap which I use to manage incoming TCP connections, it is a int64-to-Connection map read and writen by multiple go-routines.

Then I developed a remote control system based on Tao, a mobile app can control devices via this system. However I find something is wrong with ConnectionMap: some already-closed connections are not removed from this map and remain existed.

I'm not quite sure it is the reason that after a while the app can hardly connect to this sytem, but I am really confused that IS THIS THE RIGHT WAY TO WRITE A CONCURRENT MAP? IS SOMETHING WRONG WITH IT? Thank you.

type ConnectionMap struct{
  sync.RWMutex
  m map[int64]Connection
}

func NewConnectionMap() *ConnectionMap {
  return &ConnectionMap{
    m: make(map[int64]Connection),
  }
}

func (cm *ConnectionMap)Clear() {
  cm.Lock()
  cm.m = make(map[int64]Connection)
  cm.Unlock()
}

func (cm *ConnectionMap)Get(k int64) (Connection, bool) {
  cm.RLock()
  conn, ok := cm.m[k]
  cm.RUnlock()
  return conn, ok
}

func (cm *ConnectionMap)IterKeys() <-chan int64 {
  kch := make(chan int64)
  go func() {
    cm.RLock()
    for k, _ := range cm.m {
      kch<- k
    }
    cm.RUnlock()
    close(kch)
  }()
  return kch
}

func (cm *ConnectionMap)IterValues() <-chan Connection {
  vch := make(chan Connection)
  go func() {
    cm.RLock()
    for _, v := range cm.m {
      vch<- v
    }
    cm.RUnlock()
    close(vch)
  }()
  return vch
}

func (cm *ConnectionMap)Put(k int64, v Connection) {
  cm.Lock()
  cm.m[k] = v
  cm.Unlock()
}

func (cm *ConnectionMap)Remove(k int64) {
  cm.Lock()
  delete(cm.m, k)
  cm.Unlock()
}

func (cm *ConnectionMap)Size() int {
  cm.RLock()
  size := len(cm.m)
  cm.RUnlock()
  return size
}

func (cm *ConnectionMap)IsEmpty() bool {
  return cm.Size() <= 0
}

Upvotes: 0

Views: 137

Answers (1)

creker
creker

Reputation: 9570

IterKeys and IterValues can block all writers if not used properly. You're using non-buffered channels and lock the whole map until all the values are read. Remember that write lock can be aquired only after all read locks are released. Any caller that will not drain the channel will leak a goroutine with the read lock held.

There're multiple solutions I can think of:

  1. Make sure, that any caller will drain the channel as fast as possible.
  2. Don't use goroutines at all. Iterate over the map synchronously, while the read lock is held.
  3. You could make locking more granular by using two channels. One will signal that you would like to read a value, second will lock the map, read the value, unlock the map and send the value. That way caller can take as much time as he wants without locking the entire map.

I'm in favour of the second solution. It's much easier to implement properly, easier to understand and use. The first is too fragile, while the third is too complex and probably unnecessary. It will also be much slower.

Upvotes: 1

Related Questions