Race condition on token refresh

[faustikle]

I found a race condition in token refresh flow. Being more "tl;dr", you MUST use sync.Mutex (or another mutual exclusion strategy) every time you need change a struct in a multi-goroutine environment, like in an API that handle requests in separated goroutines.

I made this test to prove that. It check AutoRefreshTokenProvider and ManualRefreshTokenProvider:

package main

import (
    "errors"
    "sync"
    "testing"

    "repo.incognia.com/go/incognia"
)

var (
    installationID = "{{installationID}}"
    clientID       = "{{clientID}}"
    clientSecret   = "{{clientSecret}}"
)

func TestAutoRefresh(t *testing.T) {
    provider := incognia.NewAutoRefreshTokenProvider(incognia.NewTokenClient(&incognia.TokenClientConfig{
        ClientID:     clientID,
        ClientSecret: clientSecret,
    }))

    client, _ := incognia.New(&incognia.IncogniaClientConfig{
        ClientID:      clientID,
        ClientSecret:  clientSecret,
        TokenProvider: provider,
    })

    var wg sync.WaitGroup

    for i := 0; i < 5; i++ {
        wg.Add(1)

        go func(wg *sync.WaitGroup) {
            _, err := client.RegisterSignup(installationID, &incognia.Address{
                AddressLine: "Rua teste, 300 - Valentina, João Pessoa - PB, 05029100",
            })
            if err != nil {
                t.Error(err)
            }

            wg.Done()
        }(&wg)
    }

    wg.Wait()
}

func TestManualRefresh(t *testing.T) {
    provider := incognia.NewManualRefreshTokenProvider(incognia.NewTokenClient(&incognia.TokenClientConfig{
        ClientID:     clientID,
        ClientSecret: clientSecret,
    }))

    client, _ := incognia.New(&incognia.IncogniaClientConfig{
        ClientID:      clientID,
        ClientSecret:  clientSecret,
        TokenProvider: provider,
    })

    var wg sync.WaitGroup

    for i := 0; i < 5; i++ {
        wg.Add(1)

        go func(wg *sync.WaitGroup) {
            _, err := client.RegisterSignup(installationID, &incognia.Address{
                AddressLine: "Rua teste, 300 - Valentina, João Pessoa - PB, 05029100",
            })
            if errors.Is(err, incognia.ErrTokenExpired) || errors.Is(err, incognia.ErrTokenNotFound) {
                provider.Refresh()
            }
            if err != nil {
                t.Error(err)
            }

            wg.Done()
        }(&wg)
    }

    wg.Wait()
}

Use the flag -race in test command, like:

$ go test ./main_test.go -race

Node that the test was made with only 5 concurrent calls, in a production environment, many more will occurred.

[lucasbarross]

Nice catch!

The solution is under its way here: https://github.com/inloco/incognia-go/pull/32

[lucasbarross]

Solved! #32