Categories
c# random

Random number generator only generating one random number

820

I have the following function:

//Function to get random number
public static int RandomNumber(int min, int max)
{
    Random random = new Random();
    return random.Next(min, max);
}

How I call it:

byte[] mac = new byte[6];
for (int x = 0; x < 6; ++x)
    mac[x] = (byte)(Misc.RandomNumber((int)0xFFFF, (int)0xFFFFFF) % 256);

If I step that loop with the debugger during runtime I get different values (which is what I want).
However, if I put a breakpoint two lines below that code, all members of the mac array have equal value.

Why does that happen?

2

  • 26

    using new Random().Next((int)0xFFFF, (int)0xFFFFFF) % 256); doesn’t yield any better “random” numbers than .Next(0, 256)

    Mar 18, 2016 at 13:16

  • 2

    You may find this NuGet package helpful. It provides a static Rand.Next(int, int) method which provides static access to random values without locking or running into the seed re-use problem

    May 7, 2016 at 16:09

1130

Every time you do new Random() it is initialized using the clock. This means that in a tight loop you get the same value lots of times. You should keep a single Random instance and keep using Next on the same instance.

//Function to get a random number 
private static readonly Random random = new Random(); 
private static readonly object syncLock = new object(); 
public static int RandomNumber(int min, int max)
{
    lock(syncLock) { // synchronize
        return random.Next(min, max);
    }
}

Edit (see comments): why do we need a lock here?

Basically, Next is going to change the internal state of the Random instance. If we do that at the same time from multiple threads, you could argue “we’ve just made the outcome even more random”, but what we are actually doing is potentially breaking the internal implementation, and we could also start getting the same numbers from different threads, which might be a problem – and might not. The guarantee of what happens internally is the bigger issue, though; since Random does not make any guarantees of thread-safety. Thus there are two valid approaches:

  • Synchronize so that we don’t access it at the same time from different threads
  • Use different Random instances per thread

Either can be fine; but mutexing a single instance from multiple callers at the same time is just asking for trouble.

The lock achieves the first (and simpler) of these approaches; however, another approach might be:

private static readonly ThreadLocal<Random> appRandom
     = new ThreadLocal<Random>(() => new Random());

this is then per-thread, so you don’t need to synchronize.

24

  • 26

    As a general rule, all static methods should be made thread-safe, since it is hard to guarantee that multiple threads won’t call it at the same time. It is not usually necessary to make instance (i.e. non-static) methods thread-safe.

    Apr 20, 2009 at 12:32

  • 5

    @Florin – there is no difference re “stack based” between the two. Static fields are just as much “external state”, and will absolutely be shared between callers. With instances, there is a good chance that different threads have different instances (a common pattern). With statics, it is guaranteed that they all share (not including [ThreadStatic]).

    Apr 20, 2009 at 13:03

  • 3

    Why can’t you use lock(random)?

    Jan 31, 2014 at 21:39

  • 6

    @Dan if the object is never exposed publicly: you can. The (very theoretical) risk is that some other thread is locking on it in ways you did not expect.

    Jan 31, 2014 at 22:34

  • 4

    @smiron It’s very likely you’re simply using the random outside of a lock as well. Locking doesn’t prevent all access to what you’re locking around – it just makes sure that two lock statements on the same instance will not run concurrently. So lock (syncObject) will only help if all random.Next() calls are also within lock (syncObject). If the scenario you describe does happen even with correct lock usage, it also extremely likely happens in a single-threaded scenario (e.g. Random is subtly broken).

    – Luaan

    Apr 21, 2015 at 12:17

132

For ease of re-use throughout your application a static class may help.

public static class StaticRandom
{
    private static int seed;

    private static ThreadLocal<Random> threadLocal = new ThreadLocal<Random>
        (() => new Random(Interlocked.Increment(ref seed)));

    static StaticRandom()
    {
        seed = Environment.TickCount;
    }

    public static Random Instance { get { return threadLocal.Value; } }
}

You can use then use static random instance with code such as

StaticRandom.Instance.Next(1, 100);

0

    67

    Mark’s solution can be quite expensive since it needs to synchronize everytime.

    We can get around the need for synchronization by using the thread-specific storage pattern:

    
    public class RandomNumber : IRandomNumber
    {
        private static readonly Random Global = new Random();
        [ThreadStatic] private static Random _local;
    
        public int Next(int max)
        {
            var localBuffer = _local;
            if (localBuffer == null) 
            {
                int seed;
                lock(Global) seed = Global.Next();
                localBuffer = new Random(seed);
                _local = localBuffer;
            }
            return localBuffer.Next(max);
        }
    }
    
    

    Measure the two implementations and you should see a significant difference.

    6

    • 16

      Locks are very cheap when they aren’t contested… and even if contested I would expect the “now do something with the number” code to dwarf the cost of the lock in most interesting scenarios.

      Sep 18, 2009 at 15:57

    • 4

      Agreed, this solves the locking problem, but isn’t this still a highly complicated solution to a trivial problem: that you need to write ”two” lines of code to generate a random number instead of one. Is this really worth it to save reading one simple line of code?

      – EMP

      Apr 15, 2010 at 23:07

    • 5

      +1 Using an additional global Random instance for getting the seed is a nice idea. Note also that the code can be further simplified using the ThreadLocal<T> class introduced in .NET 4 (as Phil also wrote below).

      – Groo

      May 9, 2014 at 8:41


    • Given that _local is ThreadStatic, why do you copy it to/from var localBuffer? Is that a performance optimization? That is, is the performance of access to a ThreadStatic variable significantly more expensive than access to a regular variable? (If so, that may nullify the alleged advantage over lock, in typical situations. If not, then the code could be simplified.)

      Aug 5, 2020 at 21:36


    • @ToolmakerSteve Yes, the stack is faster that TSS. I’m not worried about the cost compared to locking as locking introduces 100’s to 1000’s of cycles. The issue with my solution is the branch introduced by the “If” statement potentially costing 100+ cycles due to the flushing of the pipeline and the instruction cache when the branch predictor gets it wrong.

      Sep 24, 2020 at 4:20