Random number generator only generating one random number

Question

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?

Vookles

using new Random().Next((int)0xFFFF, (int)0xFFFFFF) % 256); doesn't yield any better "random" numbers than .Next(0, 256) – Lydell

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 – Gripe

Answer 1

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.

Irresistible

I'm not sure this is the answer in this case, since the question states that the function is working properly when there is no breakpoints near this segment of the code. Of course I agree that he should use one instance of Random object, however I don't think this answers his question. – Cankerworm

@John - it is hard enough just trying to keep him in sight (mostly as a dot in the middle-distance). – Irresistible

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. – Irresistible

Hmm, on the contrary, I would believe that instance methods need more synchronization because most of the time they use the external state of the object they are defined in, whilst, the static methods, use local variables (stack based) that are not shared between different (possibly parallel) invocation of the method. So I think in this particular case, the sync may be ok (because it uses a shared resource), but I wouldn't make that a general, best practice, rule. – Elm

@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]). – Irresistible

Why if multiple threads are calling the static "RandomNumber" method would cause an error? – Viyella

@Viyella are you getting an error? The "lock" should prevent threads from tripping over each-other here... – Irresistible

No... I'm not getting errors at all, and I'm not using lock. Can you please elaborate some more in the answer body what it's needed? – Viyella

@Viyella do you mean: why the lock is needed here? – Irresistible

This line from MSDN should be added "Any public static (Shared in Visual Basic) members of this type are thread safe. Any instance members are not guaranteed to be thread safe." Thanks! – Viyella

@Viyella see also the "Random is not Thread Safe" in "Community Content" - messing up the internal state is very bad – Irresistible

@MarcGravell with the ThreadLocal approach is there not still the risk of two threads generating the same sequence if appRandom is accessed at the same time by those two threads? Or does some attribute of the thread contribute to the generated sequence? – Erg

@scriptfromscratch I suppose so, yet; if that matters, you could always have a synchronized master Random ... yeuch ;p – Irresistible

Why can't you use lock(random)? – Corabella

@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. – Irresistible

I used the lock mechanism and after milions of iterations the only generated random number is 0. Using ThreadLocal fixed the problem. Theoretically lock should work perfect. But something goes wrong each time after lots of iterations. Thanks – Infuriate

@Infuriate I find that quite unlikely; a repro would be very interesting – Irresistible

@Infuriate 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). – Birdseed

The lock solution is clear to me, and is mentioned on MSDN. The ThreadLocal is not clear to me: Doesn't it suffer from the exact same problem as in the OP's question? If two threads (say, a web app) create these two ThreadLocal instances within 15 milliseconds (duration mentioned on MSDN as susceptible) - don't we have again the issue that they will both generate the same sequence of numbers? (For completeness: Issue seems only applicable to full .Net Framework). Reference: docs.microsoft.com/en-us/dotnet/api/… – Webber

@MarcGravell , Can we modify it such that no two consecutive numbers have the same value ? – Edeline

@Edeline two consecutive numbers being the same is valid randomness, but sure: you could keep hold of the last value and loop until it changes - just ... with caveats: assets.amuniversal.com/321a39e06d6401301d80001dd8b71c47 – Irresistible

Idea: if anyone is concerned that Random class may not have ideal statistical properties for some seeds, yet don't want to reduce performance significantly, you could "re-seed" periodically, with seed generated by a cryptographic method. Every "N" calls, create a new Random generator with a new seed: if (--cnt <= 0) { byte[] newSeedBytes = new byte[4]; cryptoProvider.GetBytes(newSeedBytes); int newSeed = BitConverter.ToInt32(newSeedBytes, 0); random = new Random(newSeed); cnt = 100; } – Waldowaldon

Note that if you are using .Net Core Random() no longer uses current time and hence will not fail the same way: learn.microsoft.com/en-us/dotnet/api/system.random?view=net-5.0 "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." – Deadpan

Adding to Alexei Levenkov’s comment: in .NET 6 Random.Shared was introduced, which provides a thread-safe way generate random-numbers without the need to create multiple Random instances. – Curr

Answer 2

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);

Roan

Answer 3

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.

Thrice

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. – Irresistible

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? – Subantarctic

+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). – Adventist

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.) – Waldowaldon

@Waldowaldon 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. – Thrice

Ok. I'd say that is inherent to any ThreadStatic solution. (Or if use ThreadLocal, there must be a similar test internally to determine if lazy-initialization is needed.) Once we're down to a level of optimization dominated by whether branch predictor guesses right, that's quite sufficient for any purpose I have. Thanks. – Waldowaldon

Answer 4

My answer from here:

Just reiterating the right solution:

namespace mySpace
{
    public static class Util
    {
        private static Random rnd = new Random();
        public static int GetRandom()
        {
            return rnd.Next();
        }
    }
}

So you can call:

var i = Util.GetRandom();

all throughout.

If you strictly need a true stateless static method to generate random numbers, you can rely on a Guid.

public static class Util
{
    public static int GetRandom()
    {
        return Guid.NewGuid().GetHashCode();
    }
}

It's going to be a wee bit slower, but can be much more random than Random.Next, at least from my experience.

But not:

new Random(Guid.NewGuid().GetHashCode()).Next();

The unnecessary object creation is going to make it slower especially under a loop.

And never:

new Random().Next();

Not only it's slower (inside a loop), its randomness is... well not really good according to me..

Mehitable

I do not agree with the Guid case. The Random class implements a uniform distribution. Which is not the case in Guid. Guid aim is to be unique not uniformly distributed (and its implementation is most of the time based on some hardware/machine property which is the opposite of ... randomness). – Tyrontyrone

@Tyrontyrone one, I'm not sure about that. It's hash of guid that we are talking about and not the guid itself. and its pretty random. Have you tested the two? I find much more collision with Random.Next. Two, MS doesn't generate guid based on hardware property anymore. – Mehitable

if you cannot prove the uniformity of Guid generation , then it is wrong to use it as random (and the Hash would be another step away from uniformity). Likewise, collisions aren't an issue: uniformity of collision is. Concerning the Guid generation not being on hardware anymore I'm going to RTFM, my bad (any reference?) – Tyrontyrone

@Tyrontyrone I need not prove the uniformity of guid generation as I'm talking about, I repeat, about hash of guid. GetHashCode of .NET guid is based on the string representation of the guid, and not the number itself (which will be impossible anyway), and its quite random as far as I tested. No way it showed some predictable characteristic. If Hash would be another step away from uniformity, isn't it a good thing? Is uniformity a good thing in random discussion? Or did I read your "uniformity" wrongly? – Mehitable

There is two understandings of "Random": 1. lack of pattern or 2. lack of pattern following an evolution described by a probability distribution (2 included in 1). Your Guid example is correct in case 1, not in case 2. In opposite: Random class matches case 2 (thus, case 1 too). You can only replace the usage of Random by your Guid+Hash if you are not in case 2. Case 1 is probably enough to answer the Question, and then, your Guid+Hash works fine. But it is not clearly said (ps: this uniform) – Tyrontyrone

I think using Guid+Hash for the seed and then using the Random would solve cases 1 and 2. And that's what I needed which was a not time dependent generator. – Cumber

@Tyrontyrone Just for some test data, I run several batches of both Random and Guid.NewGuid().GetHashCode() through Ent (fourmilab.ch/random) and both are similarly random. new Random(Guid.NewGuid().GetHashCode()) works just as well, as does using a synchronized "master" Random to generate seeds for "child" Randoms.. Of course, it does depend on how your system generates Guids - for my system, they are quite random, and on others it may even be crypto-random. So Windows or MS SQL seems fine nowadays. Mono and/or mobile might be different, though. – Birdseed

@Birdseed I did not expect that but ok... very interesting. Thanks for the info. – Tyrontyrone

There's a subtle distinction between the behaviour of GUIDs and random numbers. GUIDs are designed to be as unique as possible, but not necessarily random. Whilst collisions can theoretically occur (i.e. getting the same GUID twice), the chance of this should be incredible small. They are conceptually the same as a sequential integer, but designed to give a (reasonably) uniform distribution across the 'value space', as opposed to an incrementing integer. Random numbers can, and should, give collisions. For random integers between 1 and 10, you'll get the same number twice one time in ten. – Awad

@EdB As I have said in comments previously, while Guid (a large number) is meant to be unique, the GetHashCode of the Guid in .NET is derived from its string representation. The output is quite random for my liking. – Mehitable

@Mehitable - I don't disagree with this. The distinction that is important here is that GUIDs are supposed to be unique with no collisions (and maybe random), whilst random numbers are expected to have collisions, for instance the series 5 2 2 7 4 5 5 is 'random' but has repeated digits. – Awad

Do not rely on Guid. Guid is not meant to be used as random in any conceivable sense. It is only and only and only and only and nothing more than only - unique. – Colloquy

You achieved multiple threads calling the same rnd instance, but this does not make it thread safe. In fact, multiple threads can be in the Next() call corrupting the internal state. When that happens your single rng will be broken and it will not recover. – Samuels

Re "I find much more collision with Random.Next" - If minimizing collision (within an int-sized value) is important, then it makes sense to me to use a cryptographic method - those are explicitly designed and tested to have best statistical properties. The statistical properties of GUID.GetHashCode() are not guaranteed. That is, GUID's could be "good" at their full size, yet imperfectly distributed when hashed. Its an unknown. Also, are you sure its only "a wee bit slower"? I would expect it to be multiple times slower. – Waldowaldon

Answer 5

I would rather use the following class to generate random numbers:

byte[] random;
System.Security.Cryptography.RNGCryptoServiceProvider prov = new System.Security.Cryptography.RNGCryptoServiceProvider();
prov.GetBytes(random);

Hartsock

I'm not one of the down-voters, but note that standard PNRG do serve a genuine need - i.e. to be able to repeatably reproduce a sequence from a known seed. Sometimes the sheer cost of a true cryptographic RNG is too much. And sometimes a crypto RNG is necessary. Horses for courses, so to speak. – Irresistible

According to the documentation this class is thread-safe, so that's something in it's favour. – Brachycephalic

What is the probability two random strings to be one and the same using that? If the string is just 3 characters I guess this will happen with high probability but what if is 255 characters length is it possible to have the same random string or is guaranteed that this can't happen from the algorithm? – Regnal

@LyubomirVelchev - It is mathematically impossible to craft a function (or a piece of hardware or even a theoretical construct) that guarantees two independently generated strings of finite length are never the same. It cannot be: there is a finite number of choices. Given n possible strings, there is - and must be - a 1/n probability of two independent strings being the same. (And yes, this implies any cryptographic scheme is not 100% safe; however if the odds of something happening twice during the lifetime of the universe is low enough ... good enough in practice.) – Waldowaldon

Joma's later answer contains a more complete code snippet based on RNGCryptoServiceProvider. See public static int Next(int min, int max) .... But for performance, modify his code to move the new out of the Next method - see my comment there. – Waldowaldon

Answer 6

1) As Marc Gravell said, try to use ONE random-generator. It's always cool to add this to the constructor: System.Environment.TickCount.

2) One tip. Let's say you want to create 100 objects and suppose each of them should have its-own random-generator (handy if you calculate LOADS of random numbers in a very short period of time). If you would do this in a loop (generation of 100 objects), you could do this like that (to assure fully-randomness):

int inMyRandSeed;

for(int i=0;i<100;i++)
{
   inMyRandSeed = System.Environment.TickCount + i;
   .
   .
   .
   myNewObject = new MyNewObject(inMyRandSeed);  
   .
   .
   .
}

// Usage: Random m_rndGen = new Random(inMyRandSeed);

Cheers.

Crural

I would move System.Environment.TickCount out of the loop. If it ticks over while you are iterating then you will have two items initialized to the same seed. Another option would be to combine the tickcount an i differently (e.g. System.Environment.TickCount<<8 + i) – Fowling

If I understand correctly: do you mean, it could happen, that "System.Environment.TickCount + i" could result the SAME value? – Crural

EDIT: Of course, no need to have TickCount inside the loop. My bad :). – Crural

The default Random() constructor calls Random(Environment.TickCount) anyway – Kentkenta

@Kentkenta - Useful observation - if only creae one global Random generator. However, if you call the default Random() constructor twice during the same tick, you will get two Random generators that each generate the exact same sequence of random numbers. Probably not what you want! The above logic (#2) uses seeds TickCount+0, TickCount+1, etc - so the generators are all different. – Waldowaldon

Answer 7

Every time you execute

Random random = new Random (15);

It does not matter if you execute it millions of times, you will always use the same seed.

If you use

Random random = new Random ();

You get different random number sequence, if a hacker guesses the seed and your algorithm is related to the security of your system - your algorithm is broken. I you execute mult. In this constructor the seed is specified by the system clock and if several instances are created in a very short period of time (milliseconds) it is possible that they may have the same seed.

If you need safe random numbers you must use the class

System.Security.Cryptography.RNGCryptoServiceProvider

public static int Next(int min, int max)
{
    if(min >= max)
    {
        throw new ArgumentException("Min value is greater or equals than Max value.");
    }
    byte[] intBytes = new byte[4];
    using(RNGCryptoServiceProvider rng = new RNGCryptoServiceProvider())
    {
        rng.GetNonZeroBytes(intBytes);
    }
    return  min +  Math.Abs(BitConverter.ToInt32(intBytes, 0)) % (max - min + 1);
}

Usage:

int randomNumber = Next(1,100);

Minny

It does not matter if you execute it millions of times, you will always use the same seed. That's not true unless you specify the seed yourself. – Chem

Fixed up. Thanks Exactly as you say LarsTech, if the same seed is always specified, the same sequence of random numbers will always be generated. In my answer I refer to the constructor with parameters if you always use the same seed. The Random class generates only pseudo random numbers. If someone finds out what seed you have used in your algorithm, it can compromise the security or randomness of your algorithm. With the RNGCryptoServiceProvider class, you can safely have random numbers. I already corrected, thank you very much for the correction. – Minny

It is excessive to call new RNGCryptoServiceProvider() on every Next. Instead, declare private static RNGCryptoServiceProvider rng = new RNGCryptoServiceProvider(); Then remove the using wrapper; simply call rng.GetNonZeroBytes(intBytes); on that static. – Waldowaldon

Re "The Random class generates only pseudo random numbers." - ALL software algorithms generate pseudo-random number sequences. True randomness requires hardware based on some physical phenomena that is considered "truly random". OTOH, cryptographic algorithms have been carefully designed (and tested) to improve the statistical distribution of the generated sequence - to avoid brute force attacks that can exploit weaknesses in simpler random generators. Even though overkill for many uses, I agree that this gives a superior statistical distribution. – Waldowaldon

Answer 8

Starting from .NET 6, the Random class is now equipped with a static property named Shared:

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

You could use it like this:

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

Accessing a thread-safe object has a small overhead, so if you are planning to generate millions of random numbers on a single thread as fast as possible, it might be preferable to create a dedicated Random instance instead of relying on the Shared.

Technical note: the Shared property is implemented cleverly by Microsoft, so that multiple threads can use it concurrently without causing contention and blocking each other. For example if one thread calls the NextBytes method to fill a large byte array with random values, all other threads are not prevented from getting their own random values while the array is filled. This is possible because each thread interacts with its own ThreadStatic instance. In other words the Random.Shared is not itself a generator of random numbers. It's a wrapper of thread-static Random instances, and delegates each method call (Next, NextDouble etc) to the instance that is dedicated for the current thread. If you are interested you can study the source code here.

Goosegog

Answer 9

Why does that happen?

As was answered before, every time you call new Random() you get new copy of Random class initialized with the same clocks (so it returns the same values).

Now, starting from .NET 6, there is easy to use and thread-safe alternative: Random.Shared

In your example you can remove at all function RandomNumber and then use following code (with the same logic, but now it works correct):

byte[] mac = new byte[6];
for (int x = 0; x < 6; ++x)
    mac[x] = (byte)(Random.Shared.Next(0, 255));

Lombardi

Answer 10

You can use code like this:

public static class ThreadSafeRandom
{
    private static readonly Random _global = new Random();
    private static readonly ThreadLocal<Random> _local = new ThreadLocal<Random>(() =>
    {
        int seed;
        lock (_global)
        {
            seed = _global.Next();
        }
        return new Random(seed);
    });

    public static Random Instance => _local.Value;
}

This code can be used as is or via the NuGet package ThreadSafeRandomizer.

EDIT: Since .NET 6.0 you can use Random.Shared.Next() instead. You can still use the above package which chooses between the above code or Random.Shared with preprocessor directives.

Anatolian

Answer 11

I use this:

int randomNumber = int.Parse(Guid.NewGuid().ToString().FirstOrDefault(Char.IsDigit).ToString().Replace("\0", "0"));

Performance: Generating 1 million random number on my PC: 711 ms.

If the Guid not contains any number (i don't know that's possible or not) then 0 will be used as the result.

Poitiers

Answer 12

I solved the problem by using the Rnd() function:

Function RollD6() As UInteger
        RollD6 = (Math.Floor(6 * Rnd())) + 1
        Return RollD6
End Function

When the form loads, I use the Randomize() method to make sure I don't always get the same sequence of random numbers from run to run.

Razorbill

This question is about C#, not Visual Basic.NET. (Although both are .NET languages, and even though it's possible, but not so trivial, to access VB functions from C#.) – Diba

Answer 13

In Visual Basic this works (probably can be translated to C#, if not a DLL reference can be a solution):

Private Function GetRandomInt(ByVal Min As Integer, ByVal Max As Integer) As Integer
     Static Generator As System.Random = New System.Random()
     Return Generator.Next(Min, Max)
End Function

Poitiers

Answer 14

There are a lot of solutions, here one: if you want only number erase the letters and the method receives a random and the result length.

public String GenerateRandom(Random oRandom, int iLongitudPin)
{
    String sCharacters = "123456789ABCDEFGHIJKLMNPQRSTUVWXYZ123456789";
    int iLength = sCharacters.Length;
    char cCharacter;
    int iLongitudNuevaCadena = iLongitudPin; 
    String sRandomResult = "";
    for (int i = 0; i < iLongitudNuevaCadena; i++)
    {
        cCharacter = sCharacters[oRandom.Next(iLength)];
        sRandomResult += cCharacter.ToString();
    }
    return (sRandomResult);
}

Brownlee

The basic problem is still the same - you're passing in a Random instance, but you're still expecting the caller to create a shared instance. If the caller creates a new instance each time and the code is executed twice before the clock changes, you'll get the same random number. So this answer still makes assumptions which could be erroneous. – Sheugh

Also, the whole point of having a method to generate random numbers is encapsulation - that the calling method doesn't have to worry about the implementation, it's only interested in getting a random number back – Sheugh

Answer 15

Always get a positive random number.

 var nexnumber = Guid.NewGuid().GetHashCode();
        if (nexnumber < 0)
        {
            nexnumber *= -1;
        }

Gradus

This code does not use the Random object as is obvious in the question and GUIDs do not aim to be technically random (as mentioned above). – Belldas