Skip to content
This repository has been archived by the owner on Aug 1, 2024. It is now read-only.

Fix misuse of System.Random in ExponentialRetry.cs #529

Conversation

jon-couloir
Copy link

Changed the random instance to be created at the class level. This prevents multiple calls to the ShouldRetry instance in the same Environment.TickCount period (~15 milliseconds) receiving the exact same GetNext() values from the random instance. This random needs to be protected with a lock, as System.Random is not threadsafe (this is a very simple threadsafe implementation, for high concurrency there are more performant implementations of threadsafe random access at the cost of higher complexity)

e.g. see https://blogs.msdn.microsoft.com/pfxteam/2009/02/19/getting-random-numbers-in-a-thread-safe-way/

Jonathan Colyer added 2 commits September 1, 2017 14:16
…events multiple calls to the ShouldRetry instance in the same Environment.TickCount period (~15 milliseconds) receiving the exact same GetNext() values from the random instance. This random needs to be protected with a lock, as System.Random is not threadsafe (this is a simple implementation, for high concurrency there are more performant implementations of threadsafe random access at the cost of higher complexity)
@Azure Azure deleted a comment from msftclas Sep 26, 2017
@@ -97,6 +98,14 @@ public bool ShouldRetry(int currentRetryCount, int statusCode, Exception lastExc
return false;
}

private double CalculateIncrement(int currentRetryCount)
{
lock (randomLock)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use

public static class StaticRandom
{
    static int seed = Environment.TickCount;

    static readonly ThreadLocal<Random> random =
        new ThreadLocal<Random>(() => new Random(Interlocked.Increment(ref seed)));

    public static int Rand()
    {
        return random.Value.Next();
    }
}

https://stackoverflow.com/questions/19270507/correct-way-to-use-random-in-multithread-application

to avoid lock?

@kfarmer-msft
Copy link
Contributor

Howdy --

Good catch!

I'll be adding this, with the ThreadLocal incrementing seed suggested by Daniel.

@kfarmer-msft
Copy link
Contributor

Please try v9.4.2

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

Successfully merging this pull request may close these issues.

5 participants