Unleash / unleash-client-go

Unleash client SDK for Go
https://docs.getunleash.io
Apache License 2.0
138 stars 55 forks source link

fix: make variant metrics concurrency safe #129

Closed sighphyre closed 1 year ago

sighphyre commented 1 year ago

This adds a mutex lock to the metrics for GetVariant, which fixes a crash bug which occurs when the `GetVariant`` method is called in a tight loop concurrently.

The toggle data that's modified in the countVariants is bound to the bucket, which is locked by the mutex, so I want to say this safe.

I haven't added a test because concurrency reasons but this can be arbitrarily reproduced with the following hacky code snippet from version 3.2.x to 3.7.2 (doesn't occur with equivalent IsEnabled calls).

timer := time.NewTimer(1 * time.Second)

for {
    <-timer.C

    go func() {
        for i := 0; i < 10000; i++ {
            variant := unleash.GetVariant("test")
            fmt.Printf("'%s' enabled? %v\n", "test", variant)
            timer.Reset(1 * time.Second)
        }
    }()

    go func() {
        for i := 0; i < 10000; i++ {
            variant := unleash.GetVariant("test")
            fmt.Printf("'%s' enabled? %v\n", "test", variant)
            timer.Reset(1 * time.Second)
        }
    }()
}
FredrikOseberg commented 1 year ago

Defer runs this function after everything has been executed and as the function exits.

On Tue, Jan 24, 2023 at 4:56 PM Gastón Fournier @.***> wrote:

@.**** commented on this pull request.

In metrics.go https://github.com/Unleash/unleash-client-go/pull/129#discussion_r1085533857 :

+

  • m.bucketMu.Lock()
  • defer m.bucketMu.Unlock()

How does this actually work? I'm used to having the unlock after the guarded portion of the code, but maybe it's the defer what makes the difference...

— Reply to this email directly, view it on GitHub https://github.com/Unleash/unleash-client-go/pull/129#pullrequestreview-1267779648, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD2WIPSEINWLQP5ODZ3N6EDWT73RTANCNFSM6AAAAAAUE2I3KM . You are receiving this because your review was requested.Message ID: @.***>