Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Random thread safety.(#502) #579

Merged
merged 1 commit into from
Dec 25, 2023
Merged

Fix Random thread safety.(#502) #579

merged 1 commit into from
Dec 25, 2023

Conversation

LeaFrock
Copy link
Collaborator

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
Copy link
Owner

H @LeaFrock

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants