CODEWITHSUNDEEP
randomf#thread-safety
Mathias
Mathias

Reputation: 15391

Is this a proper thread-safe Random wrapper?

I am fairly inexperienced with threading and concurrency; to remedy that, I am currently working for fun on implementing a random-search algorithm in F#. I wrote a wrapper around the System.Random class, following ideas from existing C# examples - but as I am not sure how I would even begin to unit test this for faulty behavior, I'd like to hear what more experienced minds have to say, and if there are obvious flaws or improvements with my code, either due to F# syntax or threading misunderstanding:

open System
open System.Threading

type Probability() =

   static let seedGenerator = new Random()

   let localGenerator = 
      new ThreadLocal<Random>(
         fun _ -> 
            lock seedGenerator (
               fun _ -> 
                  let seed = seedGenerator.Next()
                  new Random(seed)))

   member this.Draw() = 
      localGenerator.Value.NextDouble()

My understanding of what this does: ThreadLocal ensures that for an instance, each thread receives its own instance of a Random, with its own random seed provided by a common, static Random. That way, even if multiple instances of the class are created close in time, they will receive their own seed, avoiding the problem of "duplicate" random sequences. The lock enforces that no two threads will get the same seed.

Does this look correct? Are there obvious issues?

Upvotes: 4

Views: 790

Answers (4)

Tomas Petricek
Tomas Petricek

Reputation: 243096

I think your approach is pretty reasonable - using ThreadLocal gives you safe access to the Random and using a master random number generator to provide seeds means that you'll get random values even if you access it from multiple threads at similar time. It may not be random in the cryptographical sense, but should be fine for most other applications.

As for testing, this is quite tricky. If Random breaks, it will return 0 all the time, but that's just empirical experience and it is hard to say for how long you need to keep accessing it unsafely. The best thing I can suggest is to implement some simple randomness tests (some simple ones are on WikiPedia) and access your type from multiple threads in a loop - though this is still quite bad test as it may not fail every time.

Aside, you don't need to use type to encapsulate this behaviour. It can be written as a function too:

open System
open System.Threading

module Probability =

   let Draw =
     // Create master seed generator and thread local value
     let seedGenerator = new Random()
     let localGenerator = new ThreadLocal<Random>(fun _ -> 
       lock seedGenerator (fun _ -> 
         let seed = seedGenerator.Next()
         new Random(seed)))
     // Return function that uses thread local random generator
     fun () ->
       localGenerator.Value.NextDouble()

Upvotes: 9

Brian
Brian

Reputation: 118895

This feels wrong-headed. Why not just use a singleton (only ever create one Random instance, and lock it)?

If real randomness is a concern, then maybe see RNGCryptoServiceProvider, which is threadsafe.

Upvotes: 3

Ankur
Ankur

Reputation: 33657

If you really want to go with the OO approach, then your code may be fine (I won't say 'it is' fine as I am not too smart to understand OO :) ). But in case you want to go the functional way it would be as simple as something like:

type Probability = { Draw : unit -> int }

let probabilityGenerator (n:int) = 
    let rnd = new Random()
    Seq.init n (fun _ -> new Random(rnd.Next()))
    |> Seq.map (fun r -> { Draw = fun () -> r.Next() })
    |> Seq.toList

Here you can use the function probabilityGenerator to generate as much as "Porbability" type object and then distribute them to various threads which can work on them in parallel. The important thing here is that we are not introducing lock etc in the core type i.e probability and it becomes the responsibility of the consumer how they want to distribute it across threads.

Upvotes: 0

John Palmer
John Palmer

Reputation: 25516

Unless there is a performance bottleneck, I think something like

let rand = new Random()

let rnext() = 
     lock rand (
        fun () -> 
           rand.next())

will be easier to understand, but I think your method should be fine.

Upvotes: 1

Related Questions