imperugo / StackExchange.Redis.Extensions

MIT License
601 stars 179 forks source link

Fix `Random` thread safety.(#502) #579

Closed LeaFrock closed 6 months ago

LeaFrock commented 6 months ago

The round robin connection strategy will fail in a concurrent environment, because the Random.Next is not thread safe. Random will always return 0 when the internal status is broken.

For .NET 6+, Random.Shared is imported and it can be used in multi-threads scenarios.

For .NET Standard 2.1, we have different workarounds to discuss.

1、Just use a new instance each time. The problem is that, if in a very short meanwhile, the results of all randoms are all the same because the seeds are all based on the same timestamp. Also, it allocates more heap memory.

2、Use a single instance with lock. It seems to be unacceptable as it'll make GetConnection much slower.

3、Use ThreadLocal<Random>:

    private readonly ThreadLocal<Random> localRand = new(() => new Random(Guid.NewGuid().GetHashCode()));

    var nextIds = localRand.Value.Next(0, redisConfiguration.PoolSize);

And in Dispose:

    private void Dispose(bool disposing)
    {
        if (isDisposed)
            return;

        if (disposing)
        {
            // free managed resources
            foreach (var connection in connections)
                connection.Dispose();

+         localRand.Dispose();
        }

        isDisposed = true;
    }

The cost is more memory usage as we keep a Random instance for every thread visitor.

imperugo commented 6 months ago

H @LeaFrock

Thanks for the PR and the explaination. Going to merge it that looks good to me