mennanov / limiters

Golang rate limiters for distributed applications
https://godoc.org/github.com/mennanov/limiters
MIT License
459 stars 49 forks source link

fatal error: concurrent map writes #71

Closed ZanonEdoardo closed 4 months ago

ZanonEdoardo commented 4 months ago

Hello, I see an exception in map concurrent access (GetOrCreate) using limiters in our go project, using it as multi-tennat rate limiter with a dnamodb instance.

github.com/mennanov/limiters.(*Registry).GetOrCreate(0xc00041a2a0, {0xc0007d3ef0, 0x25}, 0xc0008253e8, 0x7270e00, {0x71d2de?, 0xc000c6bcb0?, 0x1fc5080?})

I use it into a gin server middleware with method RateLimit of the following struct:

func New(
    dynamoClient *dynamodb.Client,
    callForMinute int64,
    dynamodbTableName string,
    burstCapacity int64,
) rateLimiter {
    dynamoConfig := limiters.DynamoDBTableProperties{
        TableName:        dynamodbTableName,
        PartitionKeyName: "partition",
        SortKeyName:      "sort",
        SortKeyUsed:      true,
        TTLFieldName:     "ttl",
    }

    registry := limiters.NewRegistry()
    clock := limiters.NewSystemClock()

    itemSec := (float64(numberOfSecondsInAMinute) / float64(callForMinute)) * millisIntoSec
    refillRate := time.Duration(int(itemSec)) * time.Millisecond

    go func() {
        // Garbage collect the old limiters to prevent memory leaks.
        for {
            <-time.After(refillRate)
            registry.DeleteExpired(clock.Now())
        }
    }()

    return rateLimiter{
        registry:     registry,
        clock:        clock,
        dynamoClient: dynamoClient,
        dynamoConfig: dynamoConfig,
        capacity:     burstCapacity,
        refillRate:   refillRate,
    }
}

func (rateLim rateLimiter) RateLimit(ctx context.Context, client string, username string) (bool, error) {
    tokenBucketKey := fmt.Sprintf("/client/%s/user/%s", client, username)

    bucket := rateLim.registry.GetOrCreate(tokenBucketKey, func() interface{} {
        return limiters.NewTokenBucket(
            rateLim.capacity,
            rateLim.refillRate,
            limiters.NewLockNoop(),
            limiters.NewTokenBucketDynamoDB(
                rateLim.dynamoClient,
                tokenBucketKey,
                rateLim.dynamoConfig,
                rateLim.refillRate,
                false,
            ),
            limiters.NewSystemClock(), limiters.NewStdLogger())
    }, rateLim.refillRate, time.Now())

    _, err := bucket.(*limiters.TokenBucket).Limit(ctx)
    if errors.Is(err, limiters.ErrLimitExhausted) {
        return false, nil
    } else if err != nil {
        wrap := fmt.Errorf("error to update rate limit bucket (%s, %s) %w", client, username, err)
        return true, wrap
    }

    return true, nil
}

I think that problem should be in registry.go:74 r.m[key] = item where a map is access without lock. Must i maga the concurrency outside the library?

Thanks for your help.

mennanov commented 4 months ago

It should be fixed now in https://github.com/mennanov/limiters/releases/tag/v1.6.1

Thanks for reporting!