Proposal: add a static, thread-static Random.Current instance

@stephentoub

EDIT @stephentoub 02/24/2021 to updated proposal:

namespace System
{
    public class Random
    {
        public static Random Shared { get; } // instance is thread-safe, can be used from any thread
        ...
    }
}

Background and Motivation

The Random class is a very frequently used API from even non experienced developers - it's used anywhere from creating simple programs for beginners ("let's create a guessing game"), to generating random data for unit tests and benchmarks, to all sorts of other applications (eg. shuffling a music playlist). Not everyone is aware of some of the specific details of this class, and the current API surface doesn't help in this aspect, leading a number of developers to use the type incorrectly and stumbling upon seemingly weird bugs ("why are these random numbers all zeros?!").
In particular:

The Random class doesn't have static APIs, which make is less intuitive to use. Most of the time when a developer just needs a one-shot usage of an API of some class, they'd just type the type name (pun not intended) and look at the available static APIs, rather than trying to instantiate a new throwaway instance to use in that single line of code. I believe this is the same rationale that led to Add static hash helper methods #17590.
To work around 1., I've seen a number of developers just storing some Random instance into a static field. This will inevitably make the code both more verbose (need to define a field) as well as more error prone, because Random is not thread safe and will just break down if used concurrently. I've seen a few developers discovering this issue, and I'd be lying if I said I didn't make this same exact mistake as well a few years ago 😄

The proposal is to add a new static, thread-static Random instance that devs could access. This would simply return some Random instance that they could use without worrying about concurrently or creating an instance. Making the instance thread-static would allow the BCL to reuse the same Random class without the need to actually come up with a new thread-safe implementation that could be shared across threads.

Proposed API

namespace System
{
    public class Random
    {
        public static Random Current { get; }
    }
}

Here I used the Current name from conceptually similar APIs such as SynchronizationContext.Current, which specifically refers to an instance that's directly related to the current thread. Other examples can be found in APIs such as Application.Current and Window.Current, or also a similar naming scheme is also used by APIs such as DispatcherQueue.GetForCurrentThread.

Usage Examples

For one-liners:

// BEFORE
int number = new Random().Next();

// AFTER
int number = Random.Current.Next();

For cases where multiple usages would be needed:

// BEFORE
var numbers = new int[100];
var random = new Random();

foreach (ref var x in numbers.AsSpan())
{
    // Can't use the batched API if we want a given range
    x = random.Next(100);
}

// AFTER
var numbers = new int[100];

foreach (ref var x in numbers.AsSpan())
{
    x = Random.Current.Next(100);
}

Note how in the second example we no longer need to also have the extra verbosity of storing a Random instance in a local.

Alternative Designs

I had two other possible ideas:

Expose static versions of the various APIs. That won't work since you can't overload just between static/instance methods in C#, and just having different names for the static versions would be extremely awkward, so no.
Expose a thread-safe Shared instance (à la ArrayPool<T>.Shared). That would make the implementation unnecessarily more complex, as a whole new Random type would have to be written.

Risks

Technically, devs could still just get Random.Current and store it in a field, then use it concurrently. But I mean, they could already do (and in fact, they do) that today anyway. Having a static field would effectively nudge them towards not doing this at all anymore, so I think the API addition would actually reduce the overall risks related to the usage of this type.

If we're discussing updating Random, I think it may be worth looking into changing the API to include some unmanaged generic implementations, as so:

class Random
{
    public T Next<T>() where T : unmanaged;
    public T Next<T>(T max) where T : unmanaged;
    public T Next<T>(T min, T max) where T : unmanaged;
    public void Next<T>(Span<T> fill) where T : unmanaged;
}

Where Next<T>() simply returns a random T, Next<T>(Span<T> fill) fills the provided span with random Ts, and so on. This API would simplify more complex use-cases of Random, while still being easy-to-use in the common cases. It has the added benefit of unifying the surface of the API, so we can get rid of specialized functions such as NextDouble() or NextBytes().

There is a concern with use-cases attempting to randomize an arbitrary unmanaged type (i.e. not an int, uint, byte, etc). For this, it might be best to go the way of Vector<T> with an allow-list for whatever unmanaged types are justifiable. Additionally, generic specialization could be used for types like float, where filling the type with random bytes could result in NaN or infinity (which doesn't seem in the spirit of randomizing floating-point).

Edit: I've whipped up an example implementation here (missing the maxValue & minValue, maxValue overloads as of writing this edit).

This addition would require users to always evaluate the Random.Current property on every random call. However, nothing prevents them from storing that instance on a field, which can happen by accident e.g. by capturing the instance in a closure. This could result in unexpected concurrent access, also impacting unrelated code that happens to depend on Random.Current.

However, nothing prevents them from storing that instance on a field, which can happen by accident

Wouldn't the same be true for APIs such as SynchronizationContext.Current? As in, other thread-static APIs that could still technically be used incorrectly. Or is the main issue just about the fact that a dev using this wrong on one thread might cause other code running in the same thread in the future to potentially break? In that case, is the only solution to go ahead and write a fully thread-safe Random implementation to expose through a Shared instance, or would there be other alternatives?

My rationale here was mostly just to have some static helper to create random numbers, for convenience 🙂

My rationale here was mostly just to have some static helper to create random numbers, for convenience

FWIW, if all you need is some static helper for random numbers, some exist:

runtime/src/libraries/System.Security.Cryptography.Algorithms/ref/System.Security.Cryptography.Algorithms.cs

Lines 548 to 549 in ae9da64

    
           public static int GetInt32(int toExclusive) { throw null; } 
        
           public static int GetInt32(int fromInclusive, int toExclusive) { throw null; }

I feel that the RNG methods are way too heavy-weight for the use cases mentioned (they hang off of the class that's in System.Security.Cryptography and is meant for generating "secure" randomness, which has its overhead).

The proposal essentially tries to address the extremely common scenario for using Random as a static readonly singleton and make it both easier to use in general and easier to use correctly. However, it does seem like the solution with a thread-static Random instance is, perhaps, not the best one.

I think straight static methods, as proposed by @avirule, would work better.

Average java's ThreadLocalRandom.current() enjoyer itt: I've recently started building my own small things with c# after several years working with java and it appears I completely forgot that stdlib lacking such a vital feature. Took me some time refreshing my memory with the help of the docs to get couple of 42's in a threadsafe manner.

A note about methods such as this public void Next<T>(Span<T> fill) where T : unmanaged;

I would be cautious about this kind of signature, since a user may write something like:

var key = new byte[32];
Random.Current.Next(key);

And would expect to get cryptographic level randomness here.

And would expect to get cryptographic level randomness here.

Note: that is a problem with Random.NextBytes(Span<byte> buffer) already.

Next<T>(Span<T> fill)

This API would be unsafe (not type-safe) for the same reason MemoryMarshal.AsBytes<T>(Span<T>) is unsafe. See the table in #41418 for more information. To avoid the type safety issues you'd need to provide concrete implementations for all the types you care about: byte, int, double, etc.

Wouldn't the same be true for APIs such as SynchronizationContext.Current?

SynchronizationContext.Current is intended to be captured and stored into a field, then called from other random threads. SynchronizationContext instances are required to be thread-safe to support this scenario. Random instances are not guaranteed to be thread-safe.

If we had a Random.Current, perhaps it should accompany an analyzer rule that discourages storing the value into a field? Though honestly just providing static methods as suggested in another comment might end up being easier for people to reason about. Neither Random.Current nor Random.<static-method> would allow for predictability, hence code which uses them isn't testable in either case.

With #47085 providing the implementation selection infrastructure it would now be even easier to add a thread-safe implementation for Random.Current (or maybe Random.Shared to match the ArrayPool naming).

@pentp

Riding on @pentp's comment:

Random.Shared could be documented as "returns a special thread-safe Random instance." That way, if somebody did store it in a field somewhere, nothing terrible would happen. (See Steve's comment at #43887 (comment).) JIT devirtualization could also ensure that calls to Random.Shared.Next(...) wouldn't go through virtual dispatch.

Random.Shared could be documented as "returns a special thread-safe Random instance." That way, if somebody did store it in a field somewhere, nothing terrible would happen. (See Steve's comment at #43887 (comment).) JIT devirtualization could also ensure that calls to Random.Shared.Next(...) wouldn't go through virtual dispatch.

We've merged some changes to Random since then, so here's what it could look like now:
stephentoub@7fad7ee

Video

Looks good as proposed

namespace System
{
    public partial class Random
    {
        public static Random Shared { get; }
    }
}

Navigation Menu

Repository navigation

Proposal: add a static, thread-static Random.Current instance #43887

Background and Motivation

Proposed API

Usage Examples

Alternative Designs

Risks

12 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	public static int GetInt32(int toExclusive) { throw null; }
	public static int GetInt32(int fromInclusive, int toExclusive) { throw null; }

Proposal: add a static, thread-static Random.Current instance #43887

Description

Background and Motivation

Proposed API

Usage Examples

Alternative Designs

Risks

Activity

zdivelbiss commented on Oct 27, 2020

eiriktsarpalis commented on Oct 27, 2020

Sergio0694 commented on Oct 27, 2020

stephentoub commented on Oct 27, 2020

SingleAccretion commented on Oct 27, 2020

Rattenkrieg commented on Oct 27, 2020

ayende commented on Oct 28, 2020

SingleAccretion commented on Oct 28, 2020

GrabYourPitchforks commented on Oct 29, 2020

12 remaining items

pentp commented on Feb 3, 2021

GrabYourPitchforks commented on Feb 3, 2021

stephentoub commented on Feb 4, 2021

terrajobst commented on Mar 26, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions