search

Icinga / icinga-notifications

Icinga Notifications: new and improved notifications and incident management for Icinga (work in progress, not ready for production yet)
GNU General Public License v2.0
9 stars 0 forks source link

Data Race between object.FromEvent() and (*Incident).RetriggerEscalations #250

Open oxzi opened 1 month ago

oxzi commented 1 month ago 
WARNING: DATA RACE
Write at 0x00c0007f3d00 by goroutine 72:
  github.com/icinga/icinga-notifications/internal/object.FromEvent()
      /go/src/github.com/Icinga/icinga-notifications/internal/object/object.go:140 +0x22ce
  github.com/icinga/icinga-notifications/internal/incident.ProcessEvent()
      /go/src/github.com/Icinga/icinga-notifications/internal/incident/incidents.go:218 +0x1a8
  github.com/icinga/icinga-notifications/internal/icinga2.(*Launcher).launch.func1()
      /go/src/github.com/Icinga/icinga-notifications/internal/icinga2/launcher.go:132 +0x335
  github.com/icinga/icinga-notifications/internal/icinga2.(*Client).worker()
      /go/src/github.com/Icinga/icinga-notifications/internal/icinga2/client.go:560 +0xaf5
  github.com/icinga/icinga-notifications/internal/icinga2.(*Client).Process.gowrap1()
      /go/src/github.com/Icinga/icinga-notifications/internal/icinga2/client.go:586 +0x38

Previous read at 0x00c0007f3d00 by goroutine 195174:
  github.com/icinga/icinga-notifications/internal/incident.(*Incident).RetriggerEscalations.func1()
      /go/src/github.com/Icinga/icinga-notifications/internal/incident/incident.go:256 +0x148
  github.com/icinga/icinga-notifications/internal/utils.RunInTx()
      /go/src/github.com/Icinga/icinga-notifications/internal/utils/utils.go:46 +0x26b
  github.com/icinga/icinga-notifications/internal/incident.(*Incident).RetriggerEscalations()
      /go/src/github.com/Icinga/icinga-notifications/internal/incident/incident.go:255 +0xbe6
  github.com/icinga/icinga-notifications/internal/incident.(*Incident).evaluateEscalations.func1()
      /go/src/github.com/Icinga/icinga-notifications/internal/incident/incident.go:500 +0x564

Goroutine 72 (running) created at:
  github.com/icinga/icinga-notifications/internal/icinga2.(*Client).Process()
      /go/src/github.com/Icinga/icinga-notifications/internal/icinga2/client.go:586 +0x5cc
  github.com/icinga/icinga-notifications/internal/icinga2.(*Launcher).launch.gowrap1()
      /go/src/github.com/Icinga/icinga-notifications/internal/icinga2/launcher.go:149 +0x38

Goroutine 195174 (running) created at:
  time.goFunc()
      /usr/local/go/src/time/sleep.go:177 +0x54
julianbrost commented 1 month ago

Probably not just with (*Incident).RetriggerEscalations but pretty much everything accessing the Object struct. I think two regular process event requests for the same object could overlap in way showing a similar problem.

julianbrost commented 1 month ago

Note that I wouldn't necessarily try to specifically add locking to prevent this but maybe reconsider locking on a higher level. At first glance, locking on the object instead of the incident doesn't sound like a bad idea, so that for each object, at most one event is processed at once.

This has a slight overlap with #266, I wouldn't consider it the same problem and expect fixes for both in the same PR, but it should be at least be taken into account insofar to avoid making it more difficult to fix the other one with the changes for one.