Skip to content

Conversation

ysbaddaden
Copy link
Contributor

@ysbaddaden ysbaddaden commented Sep 27, 2025

See #16157 and its comments for more details.

  • Add Random.default thread local:
    • Replaces all usages of Random::DEFAULT
    • Fix: thread safety of Enumerable#sample(n) (split thread local)
    • Fix: thread safety of Crystal::System::File.mktemp (resolve thread local on each attempt)
  • Remove most usages of Random.default to internalize its usage:
    • Add Random.next_int and Random.next_bool methods that can be called directly;
    • Prefer Random::Secure.random_bytes;
    • Methods taking a Random now default to random = nil
    • Docs are fixed to state that a custom Random instance can be passed for specific control.
  • Deprecate now unused Random::DEFAULT
  • Fix: deprecated Random::DEFAULT warning on Process.new on UNIX

To be defined: we might want to keep Random.default undocumented 🤔

Closes #16157.
Depends on #16173.

This is a simple abstraction over the `ThreadLocal` annotation (for
quick access) that also injects a reference into `Thread.current` to
keep the value visible to the GC (that can't scan `ThreadLocal` values).

There is no support for destructors. When needed just use a class with a
finalizer: when the Thread is collected the thread local value will also
be collected, which will run the finalizer.
@ysbaddaden ysbaddaden self-assigned this Sep 27, 2025
@ysbaddaden ysbaddaden added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:multithreading labels Sep 27, 2025
ReferenceStorage(T) needs a concrete type (not Random) and
PCG32.new(self) requires the type to be a PCG32 instance (not any
Random).
The goal is to avoid advertising a global random instance since even as
a thread local it ain't always safe to use. We shall instead treat it as
an internal implementation detail.
Refactors the `Crystal::System::Process.fork` methods on UNIX to only
compile the call to `Random::DEFAULT` when using `Process.fork` not when
calling `Process.new` or `Process.run`.

The patch no longer hardcodes both `will_exec` paths, only the used ones
will be compiled.
@straight-shoota
Copy link
Member

We should probably extract the refactor of Crystal::System::Process.fork. This change is not directly related to the thread safety of random.
And we need it for other reasons as well. For example, we might want to use clone(2) instead of fork for spawning a new process in order to get an atomic pidfd.

@ysbaddaden
Copy link
Contributor Author

Or move to posix_spawn (and pidfd_spawn on Linux) instead 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:multithreading

Projects

Status: Review

Development

Successfully merging this pull request may close these issues.

Random::DEFAULT is thread unsafe

4 participants