fix concurrent read and wirte group error

[giant-panda666]

There is no lock when adding aggrGroup to group map. If another goroutine call Dispatcher.Groups or Dispatcher.run clear up aggrGroups, alertmanager will be panic.

[giant-panda666]

@juliusv Could you please confirm if this problem is existed?

[stuartnelson3]

Sorry for the delay, I'll look at this soon. Adding a testcase that fails because of the race detector would be helpful and prevent a regression.

[simonpasquier]

This part should probably be protected too. It might be simpler to just add a defer after the first lock (L262):

fp := groupLabels.Fingerprint()
 
d.mtx.Lock()
defer d.mtx.Unlock()

[giant-panda666]

@stuartnelson3 ok, I'll add a testcase.
@simonpasquier this is the only place to write group, so i just add lock to there. But adding a defer after the first lock is a good choice, i will change to it.

[giant-panda666]

@stuartnelson3 I'm sorry that I can't add a testcase because it's very hard to recurrent. It only occurred once in my production, and the panic reason is that concurrent read and write map.

[simonpasquier]

👍 for me. Looking at the code it is indeed quite complicated to add a test that will catch the issue in a deterministic fashion.

[stuartnelson3]

Looks good.

In general, it feels like we're throwing locks all over the place because we're directly accessing these maps. It might simplify things to have an abstraction around a map that manages this, and access that abstraction from our code.