Improve seeding in the default constructor of Random

Posted by CodesInChaos on 7/24/2011 at 1:18 PM

I know the workarounds, since I fell for it years back in Turbo Pascal. But it's an extremely common beginner mistake. I see it come up all the time in programming forums. I can probably find a dozen "`Random` is broken - it gives me the same number all the time" questions on stackoverflow.

While it's easy to workaround I see no advantage in having such a trap.

Posted by Microsoft on 7/24/2011 at 1:08 PM

If you need more randomness, I'd suggest looking into cryptographically random number generators, such as the RNGCryptoServiceProvider class in the .NET Framework.

Posted by CodesInChaos on 7/14/2011 at 1:41 PM

There are a lot of possible implementations. My main point is that almost all of them are better than the current one. Another idea:

Environment.TickCount^GetHashCode()

Which combines the hash code of the newly created instance of `Random` with the time. This should be very fast and be reasonably unique.

Posted by David A Nelson on 7/11/2011 at 10:25 AM

If you're "not creating instances of Random very often", then you are not going to run into the problem of creating Random instances with the same seed. More generally, using a Monitor lock in such a general purpose class, especially one which is already NOT thread-safe, is a bad idea. You cannot predict the required performance characteristics of any code which may use this class.

It seems to me that the problem you are trying to correct only occurs when creating multiple instances of Random in a very tight loop. There simply is no reason to do that; create on instance and re-use it, bypassing the problem entirely without requiring any changes to the class itself. This is actually the entire point of the example you cite. As noted in the following comments:

"This problem can be avoided by creating a single Random object rather than multiple ones."

Posted by CodesInChaos on 7/11/2011 at 3:27 AM

@David
For example one could implement it like the following. The performance cost of locking shouldn't be significant since you shouldn't create instances of `Random` very often. And existing code that generates multiple `Random` instances from the parameterless constructor within a Millisecond is already broken.

private static object _lock=new object();
private static Random _seedGenerator;

private static int GenerateSeed()
{
    lock(_lock)
    {
        if(_seedGenerator==null)
            _seedGenerator=new Random(Environment.TickCount);
        return _seedGenerator.Next();
    }
}

public Random()
    :this(GenerateSeed)
{
}

Posted by Microsoft on 7/7/2011 at 9:13 AM

Thank you for your feedback, we are currently reviewing the issue you have submitted. If this issue is urgent, please contact support directly(http://support.microsoft.com)

Posted by David A Nelson on 7/6/2011 at 9:14 AM

What alternative would you propose?

Improve seeding in the default constructor of Random by CodesInChaos