Skip to content

Add thread safety to default random#16174

Merged
straight-shoota merged 13 commits intocrystal-lang:masterfrom
ysbaddaden:feature/thread-safe-default-random
Nov 14, 2025
Merged

Add thread safety to default random#16174
straight-shoota merged 13 commits intocrystal-lang:masterfrom
ysbaddaden:feature/thread-safe-default-random

Conversation

@ysbaddaden
Copy link
Collaborator

@ysbaddaden ysbaddaden commented Sep 27, 2025

See #16157 and its comments for more details.

  • Add Random.default thread local to replace all usages of Random::DEFAULT.
  • Fix thread safety of Enumerable#sample(n) by splitting the thread local to a new random sequence.
  • Deprecate now unused Random::DEFAULT.

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

Depends on:

Closes #16157.

@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
@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
Collaborator Author

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

@ysbaddaden ysbaddaden marked this pull request as ready for review October 28, 2025 17:03
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite a big patch. We should probably split it down a bit more into smaller, self-contained change sets.

The change for internalizing Random.default could each already be an individual PR:

  • Change method signatures to random = nil and update docs
  • Add Random.next_int and Random.next_bool
  • Prefer Random::Secure.random_bytes

@ysbaddaden
Copy link
Collaborator Author

Rebased from master with extracted commits squashed back in.

@ysbaddaden ysbaddaden marked this pull request as draft October 30, 2025 17:04
@ysbaddaden ysbaddaden force-pushed the feature/thread-safe-default-random branch from e1a783a to cf62e67 Compare October 30, 2025 17:05
@ysbaddaden ysbaddaden marked this pull request as ready for review November 1, 2025 19:52
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a leftover DEFAULT reference in #next_int.

@ysbaddaden
Copy link
Collaborator Author

Extracted def from macro and undocumented Random.default for the time being.

We might want to formalize an API to "split a random" with a couple #split and #split! methods, though. For example xoshiro, splitmix and Java's SplittableRandom all have the ability to split. In that case we might want to extract yet-another PR 😅

straight-shoota pushed a commit that referenced this pull request Nov 9, 2025
…6342)

Adds a couple methods to formalize the API for splitting a `Random` instance:

- `Random#split`: splits the instance by allocating a new instance.
- `#split_internal`: splits the instance into an allocated instance (for example `ReferenceStorage(T)`).

I initially wanted a `#split!` method with no argument, but we need access to both instances to avoid splits (and re-splits of splits) to end up sharing the same sequences.

Extracted from #16174 and adapted with usages in the [random shard](ysbaddaden/random.cr#1, XoShiRo).
@ysbaddaden ysbaddaden force-pushed the feature/thread-safe-default-random branch from 16e24a7 to bc2c995 Compare November 13, 2025 14:07
@ysbaddaden
Copy link
Collaborator Author

Final rebase on master to bring the reworked and now official #split and #split_internal methods. That simplifies the macro as .split_on_stack only, since the whole behavior is now bundled into the split methods.

I renamed the TLS as Random.thread_default.

Co-authored-by: Johannes Müller <straightshoota@gmail.com>
@straight-shoota straight-shoota added this to the 1.19.0 milestone Nov 13, 2025
@straight-shoota straight-shoota merged commit 360ce33 into crystal-lang:master Nov 14, 2025
40 checks passed
@github-project-automation github-project-automation bot moved this from Review to Done in Multi-threading Nov 14, 2025
@ysbaddaden ysbaddaden deleted the feature/thread-safe-default-random branch November 15, 2025 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Random::DEFAULT is thread unsafe

4 participants