Wanted data-race or bad design?

Question

I am implementing an app that integrates a third party API that has a limit of hits per second. I wrote my adapter and I was a happy man until I run my tests with the race condition detector.

The design is simple, there is a:

A struct that counts the requests it has made
A tick that resets this counter to 0 every second
A private function on this struct which is blocking until conditions are met to allow to do an extra call to the API.

Running this test case works very well until you give it the -race flag. I believe the data-race is caused by the tick thread trying to reset the hit counter and the call requests who increments it...

Is my design bad or should I just live with a data-race alert ?


import (
    "sync"
    "testing"
    "time"
)

var subject httpClientWrapper

func init() {
    subject = httpClientWrapper{
        hits:       0,
        hitsSecond: 1,
    }
    // reset hits every second to 0
    go func() {
        tick := time.Tick(1 * time.Second)
        for range tick {
            subject.hits = 0
        }
    }()
}

type httpClientWrapper struct {
    hits, hitsSecond int
}

var m sync.Mutex

func (c *httpClientWrapper) allowCall() {
    m.Lock()
    callAllowanceReached := c.hits >= c.hitsSecond
    for callAllowanceReached {
        // cool down for one second
        time.Sleep(1 * time.Second)
        callAllowanceReached = c.hits >= c.hitsSecond
    }
    c.hits = c.hits + 1
    m.Unlock()
}

func TestItSleeps(t *testing.T) {
    timeStart := time.Now()
    var wg = sync.WaitGroup{}
    for i := 0; i < 3; i++ {
        wg.Add(1)
        go func() {
            subject.allowCall()
            wg.Done()
        }()
    }
    wg.Wait()
    elapsedTime := time.Since(timeStart)
    if elapsedTime < (1 * time.Second) {
        t.Errorf("this test should not had been able to run in less than a second due to locks and cool down")
    }
}

Never just "live with" a race condition. – Adrian Dec 30 '19 at 04:42 — Adrian, Dec 30 '19 at 04:42

score 1 · Accepted Answer · edited Dec 30 '19 at 13:02

Any access to .hits should be behind the mutex, so

// reset hits every second to 0
go func() {
    tick := time.Tick(1 * time.Second)
    for range tick {
        m.Lock()
        subject.hits = 0
        m.Unlock()
    }
}()

Also any sleeps should not occur with the mutex locked, so

m.Lock()
...
    {
        m.Unlock()
        // cool down for one second
        time.Sleep(1 * time.Second)
        m.Lock()
        ...
    }
...
m.Unlock()

Wanted data-race or bad design?

1 Answers1