3

I am using C# Random to generate random numbers in a multi threaded scenario. According to the recommendation here by microsoft, I am using a static shared Random object for all the callers and is calling random.Next in a lock.


        private static TimeSpan GetRandomJitter(IReadOnlyCollection<TimeSpan> timeSpans)
        {
            lock (s_randomizerLock)
            {
                int random = s_randomizerAcrossTenants.Next(timeSpans.Count);
                return timeSpans.ElementAt(random);
            }
        }

But in the example Microsoft provided in the same page, They are using an additional check to see if the random is corrupted even then they have a lock for the usages of Next()


private void GetRandomNumbers(Object o)
   {
      CancellationToken token = (CancellationToken) o;
      double result = 0.0;
      countdown.AddCount(1);

      try {
         for (int ctr = 0; ctr < 2000000; ctr++)
         {
            // Make sure there's no corruption of Random.
            token.ThrowIfCancellationRequested();

            lock (randLock) {
               result = rand.NextDouble();
            }
            // Check for corruption of Random instance.
            if ((result == previous) && result == 0) {
               source.Cancel();
            }
            else {
               previous = result;
            }
            perThreadCtr++;
            perThreadTotal += result;
         }

         Console.WriteLine("Thread {0} finished execution.",
                           Thread.CurrentThread.Name);
         Console.WriteLine("Random numbers generated: {0:N0}", perThreadCtr);
         Console.WriteLine("Sum of random numbers: {0:N2}", perThreadTotal);
         Console.WriteLine("Random number mean: {0:N4}\n", perThreadTotal/perThreadCtr);

         // Update overall totals.
         lock (numericLock) {
            totalCount += perThreadCtr;
            totalValue += perThreadTotal;
         }
      }
      catch (OperationCanceledException e) {
         Console.WriteLine("Corruption in Thread {1}", e.GetType().Name, Thread.CurrentThread.Name);
      }
      finally {
         countdown.Signal();
      }
   }
}

Why would they need a check for corruption? What might be a real life case for checking for Random corruption when I can make sure that all the usages to Random.Next() is locked by a randomizer lock? How do we mitigate if my shared/static Random get corrupted?

Is there any recommendation around this?

Fildor
  • 14,510
  • 4
  • 35
  • 67
Jins Peter
  • 2,368
  • 1
  • 18
  • 37
  • 1
    You might have all sorts of problems if some other function uses the same `Random` instance and forgets to use the lock. Beyond that...? The only conclusion is that this particular `Random` class is hopelessly broken and you should use a different one. – Ben Voigt May 16 '23 at 20:36
  • 1
    I dont really need a real real random. But I dont want Random.Next() to return me 0 every time too. I can make sure that no other method will be using Random.Next without a lock. In that case, is there a problem? – Jins Peter May 16 '23 at 20:40
  • 1
    It looks to me that is it is just checking code -- lets put this in to see if something when wrong -- we don't think there is any way for something to go wrong but we will check anyway. -- then nothing went wrong but they didn't take the test code out. – Hogan May 16 '23 at 20:41
  • "The Random() constructor uses the default seed value. This is the most common way of instantiating the random number generator. In .NET Framework, the default seed value is time-dependent. In .NET Core, the default seed value is produced by the thread-static, pseudo-random number generator." That's what I would do -- create a separate `Random()` instance for each thread but seed them all with output from a shared (under lock) `Random()`. That way you avoid the problem of seeding multiple objects from the system clock at the same time. – Ben Voigt May 16 '23 at 20:44
  • 1
    Yes I read the code -- that check is there to make sure if something goes wrong it will stop and clean up the threads in a nice way. That will never happen -- as you cans see from the output the example ran 10 million times without issue. – Hogan May 16 '23 at 20:46
  • @BenVoigt. Yeah. But that is about getting better random numbers as output. That is not really my use case. I dont really need a super good random number. But I just dont want a corrupt Random obj to keep returning 0 like the document says. – Jins Peter May 16 '23 at 20:46
  • 1
    It is not when it is corrupt that it returns 0 it is when you don't use a lock that it will return 0 – Hogan May 16 '23 at 20:47
  • 1
    Looks like garbage to me. I usually just lock around static Random class. – eocron May 16 '23 at 20:48
  • 1
    @JinsPeter: Well, my suggestion was actually about eliminating contention for the lock. If the shared RNG is only used for seeds, not for every single random number, then contention will be greatly reduced. And the not-shared RNGs don't have any concern of corruption. – Ben Voigt May 16 '23 at 20:48
  • 2
    _"Why would they need a check for curruption?"_ - for the sake of example? To show that no corruption is happening? – Guru Stron May 16 '23 at 20:48
  • @Hogan "as you cans see from the output the example ran 10 million times without issue.". Got that now. Thanks – Jins Peter May 16 '23 at 20:51
  • 3
    Note that since .NET 6 you can just use [`Random.Shared`](https://learn.microsoft.com/en-us/dotnet/api/system.random.shared?view=net-7.0) which is _"a thread-safe Random instance that may be used concurrently from any thread."_ – Guru Stron May 16 '23 at 20:52
  • @GuruStron The Random.Shared.Next() Call still isn't thread safe? Isn't it is just a helper property to use app wide Randomizer? – Jins Peter May 16 '23 at 20:57
  • 2
    @JinsPeter The previous quote is taken from the docs. _"The Random.Shared.Next() Call still isn't thread safe?"_ - why do you think so? _"Isn't it is just a helper property to use app wide Randomizer? "_ - not, it returns [`ThreadSafeRandom`](https://source.dot.net/System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/Random.cs.html#https://source.dot.net/System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/Random.cs.html,f96d9b9c631fb037) which internally uses thread static instances. – Guru Stron May 16 '23 at 20:59
  • @GuruStron because, MS documentation is talking only about using Random.Next() in a lock. – Jins Peter May 16 '23 at 21:04
  • 1
    @GuruStron Just looked into the .NET 6 code. It looks like it is safe to use `Random.Shared.Next()` any way we want. Random.Next itself is virtual and Random.Shared is a `ThreadSafeRandom` type. The Next method is overriden there to allow concurrent usages. ie. You are right. – Jins Peter May 16 '23 at 21:05
  • 1
    @JinsPeter Microsoft docs evolved for quite many years, I would expect that they will have some inconsistencies. Again, docs claim `Random.Shared` to be thread safe *instance*, so it's members should be threadsafe. _"Just looked into the...."_ - yep, exactly what I have written on my previous comment. – Guru Stron May 16 '23 at 21:07
  • Nowhere is explained how a Random instance can become "corrupt" and why you can detect said corruption by it returning a 0 twice in a row. It's probably a leftover from 20+ year old example documentation. – CodeCaster May 16 '23 at 21:35
  • @CodeCaster it seems that the zeroes can be still present - try running [this .NET 5 fiddle](https://dotnetfiddle.net/DgEE47). Some runs produce too much zeroes. Note that now for seeded random something called `Net5CompatSeedImpl`/`Net5CompatDerivedImpl` [is used](https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/Random.cs,37) – Guru Stron May 16 '23 at 22:11
  • 1
    @Guru that's not documented, but observed. Just wondering why. – CodeCaster May 17 '23 at 04:08

1 Answers1

3

Based on the next quotes:

The following example uses the C# lock Statement ... to ensure that a single random number generator is accessed by 11 threads in a thread-safe manner.

and

The example checks whether the random number generator has become corrupted by determining whether two consecutive calls to random number generation methods return 0. If corruption is detected, the example uses the CancellationTokenSource object to signal that all threads should be canceled.

My guess would be that the part in question was added for demonstration purposes to show that no corruption is happening.

P.S.

If you can migrate to .NET 6+ - just use Random.Shared:

Provides a thread-safe Random instance that may be used concurrently from any thread.

Currently it is implemented via special private ThreadSafeRandom class which handles the thread safety via [ThreadStatic] instances to prevent need for locking.

Guru Stron
  • 102,774
  • 10
  • 95
  • 132