Only reset the seed in Generator::__destruct() if it is this generator's - #1074
Open
TheoGibbons wants to merge 1 commit into
Open
TheoGibbons wants to merge 1 commit into
TheoGibbons wants to merge 1 commit into
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the reason for this PR?
Generator::__destruct()callsmt_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 calledseed(), and ones whose seed has since been replaced by another generator's. Every generatorFactory::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 newFactory::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).Generator::seedinGenerator::__destructmay break determinism #870)Author's checklist
Summary of changes
seed()records which generator set the current seed, in a private staticWeakReference.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.mt_rand().WeakReferenceneeds PHP 7.4, which matchescomposer.json. I checked thatWeakReference::get()still returns$thisinside__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:
GeneratorTest. Three of them fail on1.24without this change, including theGenerator::seedinGenerator::__destructmay break determinism #870 scenario, reproduced withgc_collect_cycles().MT_RAND_PHPandMT_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 needext-intl, which the plainphp:*-cliDocker images I ran them in don't have.This targets
1.24.2.0has the same destructor insrc/Generator.php, and I'm happy to open the same change there.Review checklist
CHANGELOG.md