0

I'm pretty new in Go and need the answers to some of dilemmas while implementing small HTTP notifier than runs concurrently, sending message to the configured HTTP endpoint. For that purpose, I use the following structure:

type Notifier struct {
    url          string
    waitingQueue *list.List
    progress   *list.List

    errs chan NotificationError

    active sync.WaitGroup
    mu     sync.Mutex
}

Here is the notify method:

func (hn *Notifier) Notify(message string) {
    if hn.ProcessingCount() < hn.config.MaxActiveRequestsCount() && hn.checkIfOlderThanOldestWaiting(time.Now().Unix()) {
        element := hn.addToProgressQueue(message)
        hn.active.Add(1)
        go hn.sendNotification(element)
    } else {
        hn.waitingQueue.PushBack(QueueItem{message, time.Now().UnixNano()})
    }
}

And addToInProgressQueue:

func (hn *Notifier) addToProgressQueue(message string) *list.Element {
    hn.mu.Lock()
    defer hn.mu.Unlock()

    queueItem := QueueItem{message, time.Now().UnixNano()}
    element := hn.progress.PushBack(queueItem)
    return element
}

  1. I guess this won't work as expected for concurrent reads/writes of queue? Is it enought to use RWMutex instead of Mutex to ensure the locks are working properly?

  2. The code of ProcessingCount

func (hn *Notifier) ProcessingCount() int {
    hn.mu.Lock()
    defer hn.mu.Unlock()

    return hn.inProgress.Len()
}

Can there be a data race here?

Also, if you have some good resources on data race examples in Golang, it would be well appreciated.

Thanks in advance

JimB
  • 104,193
  • 13
  • 262
  • 255
aldm
  • 343
  • 1
  • 11
  • 3
    It's enough to use `sync.Mutex`. The difference of `sync.RWMutex` is that the latter allows multiple "readers" and a single writer, while the former only allows a single "accessor". That is, "readers" and "writers" are merely convenient terms, simply because it has no sense to have multiple writers and a single reader. Note that `RWMutex` is only needed if you have the estimated rate of read access to be way higher than that of write accesses. – kostix Mar 29 '22 at 13:57
  • View `RWMutex` only as an optimization. I typically always start with a `Mutex` to be sure the locking strategy is correct to begin with. – JimB Mar 29 '22 at 13:58
  • 1) No. You're writing here, so using an RWMutex won't change anything, you can't use a read lock here. 2) Sure, there can be if anything is writing to `hn.inProgress` *without* locking the mutex. This seems like a really complicated approach to something that sounds relatively simple though, I'm not sure all of this is necessary just to call a webhook. – Adrian Mar 29 '22 at 13:58
  • 1
    As to a data race, I'd say the main question which remains, is: is `ProcessingCount` the only _other_ place where the queue is accessed (with the first place being `addToProgressQueue`)? I'd expect the queue to be drained, and this means accessing it, and this means it needs to be locked as well. – kostix Mar 29 '22 at 13:58
  • 1
    And a one note on style: if a mutex protects not all the members of a `struct`, place it right above the members it protects (and possibly place a single empty line above it—to clearly delineate the group of fields). – kostix Mar 29 '22 at 13:59
  • could not you use a simple channel ? At least it would force you to cap the maximum concurrent items to process. Also, considering list.List is unsafe for concurrent use, and that the notifier.Notify is called in parallel, the call to waitingQueue.PushBack is racy imo. Anyways, the race detector will tell you all if you dare using it. https://go.dev/blog/race-detector –  Mar 29 '22 at 19:50
  • Thanks very much for the answers guys. Yes, there is a bunch of other functions that access to `hn. inProgress`, so I'll need to check all the locks. I will check data race with race detector. Regarding the complexity, there is also a requirement to secure a graceful shutdown, i.e. wait for all queued items to be sent, so that's why I decided for this approach. – aldm Mar 30 '22 at 07:20

0 Answers0