Skip to content

Only reset the seed in Generator::__destruct() if it is this generator's - #1074

Open
TheoGibbons wants to merge 1 commit into
FakerPHP:1.24from
TheoGibbons:fix/destruct-only-resets-own-seed
Open

TheoGibbons wants to merge 1 commit into
FakerPHP:1.24from
TheoGibbons:fix/destruct-only-resets-own-seed

Conversation

@TheoGibbons

Copy link
Copy Markdown

What is the reason for this PR?

Generator::__destruct() calls mt_srand() so that a seed does not outlive the generator that set it (fzaninotto#1534). But it does that for every generator: ones that never called seed(), and ones whose seed has since been replaced by another generator's. Every generator Factory::create() builds references itself through its providers, so it is only destroyed when PHP's cycle collector next runs. That timing depends on memory use, not on the code. An old, unreachable generator can therefore replace a newer seed with a random one at any point, and a seeded run's output changes when unrelated code changes.

This is the problem reported in #870 (closed without a fix). We hit it through Scribe's examples.faker_seed, which builds a new Factory::create() generator for each example: our generated API docs had a different example email on each of our last 25 CI runs. With this patch applied to Faker, the same docs come out byte-identical however the collector's timing is shifted (without it, 24–48 lines changed from run to run).

Author's checklist

Summary of changes

  • seed() records which generator set the current seed, in a private static WeakReference. seed() with no argument clears it.
  • __destruct() restores a random seed only when the generator being destroyed is the one that set it. The guarantee from Restore a random seed when the Generator is destroyed fzaninotto/Faker#1534, that a seed does not outlive its generator, is unchanged and now has a test.
  • Destroying a generator that never seeded, or whose seed has since been replaced, no longer touches mt_rand().

WeakReference needs PHP 7.4, which matches composer.json. I checked that WeakReference::get() still returns $this inside __destruct() on PHP 7.4 and 8.4, both when the object is released normally and when the cycle collector frees it.

One behaviour change: code that relied on destroying an unrelated generator to re-randomise mt_rand() no longer gets that. Calling $faker->seed() does it explicitly.

Tests:

  • Four new tests in GeneratorTest. Three of them fail on 1.24 without this change, including the Generator::seed in Generator::__destruct may break determinism #870 scenario, reproduced with gc_collect_cycles().
  • Full suite on PHP 7.4, 8.2 and 8.4 (covering both MT_RAND_PHP and MT_RAND_MT19937): the same results with and without the change, apart from those three tests. 25 errors appear either way. They are locale tests that need ext-intl, which the plain php:*-cli Docker images I ran them in don't have.
  • phpstan and psalm report nothing new, and php-cs-fixer is clean.

This targets 1.24. 2.0 has the same destructor in src/Generator.php, and I'm happy to open the same change there.

Review checklist

  • All checks have passed
  • Changes are added to the CHANGELOG.md
  • Changes are approved by maintainer

Since fzaninotto#1534 (fzaninotto/Faker), destroying any Generator calls mt_srand()
so that a seed does not outlive the generator that set it. But the
destructor runs for every generator, seeded or not, and a generator that
references itself through its providers (every one Factory::create()
builds) is only destroyed when the cycle collector next runs. When that
happens after another generator has been seeded, it silently replaces
that newer seed with a random one, at a point that depends on memory
pressure rather than on the code (FakerPHP#870).

Record which generator set the current seed, and only restore a random
seed when that generator is the one being destroyed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant