2

I have stared myself blind on this now, so please help.

When I call this method twice, inside a loop, it returns the same values. Why ?

public async Task<int> RollDice() {
    var rnd = new Random();
    var selected = 0;

    await Task.Run(() => {
        selected = rnd.Next(1, 6);
        });

    return selected;
}
John Willemse
  • 6,608
  • 7
  • 31
  • 45
danielovich
  • 9,217
  • 7
  • 26
  • 28

2 Answers2

7

You have to initialize the Random object outside of the method to prevent it from being initialized over and over again with the same seed and therefore returning the same values.

It is important to notice, as LukeH correctly adds in the comments, that the System.Random class is not thread safe and should not be shared among tasks in separate threads.

John Willemse
  • 6,608
  • 7
  • 31
  • 45
  • 2
    Correct, but... beware that `System.Random` isn't thread-safe, so sharing a single instance between multiple threadpool tasks (via `Task.Run`) isn't a good idea. – LukeH Apr 11 '13 at 09:03
  • 1
    I edited the code so the random is a static. private static Random rnd = new Random(); public async Task RollDice() { var selected = 0; await Task.Run(() => { selected = rnd.Next(1, 6); }); return selected; } – danielovich Apr 11 '13 at 09:07
  • 2
    The lack of thread safety might cause this class to produce random results... oh, wait :P – Thorarin Apr 11 '13 at 09:07
  • 1
    Seriously through @danielovich, the revised code you posted not recommendable, because of the thread-safety issues. See the article linked in my answer for a workaround. – Thorarin Apr 11 '13 at 09:22
  • so I will add a lock. Thx @Thorarin – danielovich Apr 11 '13 at 09:37
2

You're using two instances of the Random class. Using the parameterless constructor, the the random number generator is seeded using a system clock based value.

This means that if you're creating two Random instances shortly after each other, they will be initialized using the same value, because the system clock has a finite resolution. Having the same seed means that these two instances will produce the same sequence of results.

Some Googling revealed that using Random cross-thread can cause it to break and return an endless sequence of zeros, so if you have to multi-thread this particular part of the code, you may want to look at this article on how to create a single thread safe version of the Random class.

Thorarin
  • 47,289
  • 11
  • 75
  • 111