improve hash randomizer

Co-authored-by: Tim Düsterhus <tim@bastelstu.be>
symfony · Apr 24, 2023 · 8b84396 · 8b84396
1 parent 8dd016a
commit 8b84396
Show file tree

Hide file tree

Showing 3 changed files with 64 additions and 46 deletions.
diff --git a/src/Symfony/Component/Scheduler/Tests/RecurringMessageTest.php b/src/Symfony/Component/Scheduler/Tests/RecurringMessageTest.php
@@ -29,10 +29,10 @@ public function __toString(): string
 
         if (class_exists(Randomizer::class)) {
             $this->assertSame('30 0 * * *', (string) RecurringMessage::cron('#midnight', $object)->getTrigger());
-            $this->assertSame('30 6 * * 2', (string) RecurringMessage::cron('#weekly', $object)->getTrigger());
+            $this->assertSame('30 0 * * 3', (string) RecurringMessage::cron('#weekly', $object)->getTrigger());
         } else {
-            $this->assertSame('56 2 * * *', (string) RecurringMessage::cron('#midnight', $object)->getTrigger());
-            $this->assertSame('56 20 * * 0', (string) RecurringMessage::cron('#weekly', $object)->getTrigger());
+            $this->assertSame('36 0 * * *', (string) RecurringMessage::cron('#midnight', $object)->getTrigger());
+            $this->assertSame('36 0 * * 6', (string) RecurringMessage::cron('#weekly', $object)->getTrigger());
         }
     }
 

diff --git a/src/Symfony/Component/Scheduler/Tests/Trigger/CronExpressionTriggerTest.php b/src/Symfony/Component/Scheduler/Tests/Trigger/CronExpressionTriggerTest.php
@@ -36,54 +36,61 @@ public static function hashedExpressionProvider(): array
         if (class_exists(Randomizer::class)) {
             return [
                 ['# * * * *', '30 * * * *'],
-                ['# # * * *', '30 6 * * *'],
-                ['# # # * *', '30 6 3 * *'],
-                ['# # # # *', '30 6 3 7 *'],
-                ['# # # # #', '30 6 3 7 2'],
-                ['# # 1,15 1-11 *', '30 6 1,15 1-11 *'],
-                ['# # 1,15 * *', '30 6 1,15 * *'],
+                ['# # * * *', '30 0 * * *'],
+                ['# # # * *', '30 0 25 * *'],
+                ['# # # # *', '30 0 25 10 *'],
+                ['# # # # #', '30 0 25 10 5'],
+                ['# # 1,15 1-11 *', '30 0 1,15 1-11 *'],
+                ['# # 1,15 * *', '30 0 1,15 * *'],
                 ['#hourly', '30 * * * *'],
-                ['#daily', '30 6 * * *'],
-                ['#weekly', '30 6 * * 2'],
-                ['#weekly@midnight', '30 0 * * 2'],
-                ['#monthly', '30 6 3 * *'],
-                ['#monthly@midnight', '30 0 3 * *'],
-                ['#yearly', '30 6 3 7 *'],
-                ['#yearly@midnight', '30 0 3 7 *'],
-                ['#annually', '30 6 3 7 *'],
-                ['#annually@midnight', '30 0 3 7 *'],
+                ['#daily', '30 0 * * *'],
+                ['#weekly', '30 0 * * 3'],
+                ['#weekly@midnight', '30 0 * * 3'],
+                ['#monthly', '30 0 25 * *'],
+                ['#monthly@midnight', '30 0 25 * *'],
+                ['#yearly', '30 0 25 10 *'],
+                ['#yearly@midnight', '30 0 25 10 *'],
+                ['#annually', '30 0 25 10 *'],
+                ['#annually@midnight', '30 0 25 10 *'],
                 ['#midnight', '30 0 * * *'],
                 ['#(1-15) * * * *', '1 * * * *'],
                 ['#(1-15) * * * #(3-5)', '1 * * * 3'],
-                ['#(1-15) * # * #(3-5)', '1 * 3 * 3'],
+                ['#(1-15) * # * #(3-5)', '1 * 17 * 5'],
             ];
         }
 
         return [
-            ['# * * * *', '56 * * * *'],
-            ['# # * * *', '56 20 * * *'],
-            ['# # # * *', '56 20 1 * *'],
-            ['# # # # *', '56 20 1 9 *'],
-            ['# # # # #', '56 20 1 9 0'],
-            ['# # 1,15 1-11 *', '56 20 1,15 1-11 *'],
-            ['# # 1,15 * *', '56 20 1,15 * *'],
-            ['#hourly', '56 * * * *'],
-            ['#daily', '56 20 * * *'],
-            ['#weekly', '56 20 * * 0'],
-            ['#weekly@midnight', '56 2 * * 0'],
-            ['#monthly', '56 20 1 * *'],
-            ['#monthly@midnight', '56 2 1 * *'],
-            ['#yearly', '56 20 1 9 *'],
-            ['#yearly@midnight', '56 2 1 9 *'],
-            ['#annually', '56 20 1 9 *'],
-            ['#annually@midnight', '56 2 1 9 *'],
-            ['#midnight', '56 2 * * *'],
-            ['#(1-15) * * * *', '12 * * * *'],
-            ['#(1-15) * * * #(3-5)', '12 * * * 5'],
-            ['#(1-15) * # * #(3-5)', '12 * 1 * 5'],
+            ['# * * * *', '36 * * * *'],
+            ['# # * * *', '36 0 * * *'],
+            ['# # # * *', '36 0 14 * *'],
+            ['# # # # *', '36 0 14 3 *'],
+            ['# # # # #', '36 0 14 3 5'],
+            ['# # 1,15 1-11 *', '36 0 1,15 1-11 *'],
+            ['# # 1,15 * *', '36 0 1,15 * *'],
+            ['#hourly', '36 * * * *'],
+            ['#daily', '36 0 * * *'],
+            ['#weekly', '36 0 * * 6'],
+            ['#weekly@midnight', '36 0 * * 6'],
+            ['#monthly', '36 0 14 * *'],
+            ['#monthly@midnight', '36 0 14 * *'],
+            ['#yearly', '36 0 14 3 *'],
+            ['#yearly@midnight', '36 0 14 3 *'],
+            ['#annually', '36 0 14 3 *'],
+            ['#annually@midnight', '36 0 14 3 *'],
+            ['#midnight', '36 0 * * *'],
+            ['#(1-15) * * * *', '7 * * * *'],
+            ['#(1-15) * * * #(3-5)', '7 * * * 3'],
+            ['#(1-15) * # * #(3-5)', '7 * 1 * 5'],
         ];
     }
 
+    public function testHashFieldsAreRandomizedIndependently()
+    {
+        $parts = explode(' ', (string) CronExpressionTrigger::fromSpec('#(1-6) #(1-6) #(1-6) #(1-6) #(1-6)', 'some context'));
+
+        $this->assertNotCount(1, array_unique($parts));
+    }
+
     public function testFromHashWithStandardExpression()
     {
         $this->assertSame('56 20 1 9 0', (string) CronExpressionTrigger::fromSpec('56 20 1 9 0', 'some context'));

diff --git a/src/Symfony/Component/Scheduler/Trigger/CronExpressionTrigger.php b/src/Symfony/Component/Scheduler/Trigger/CronExpressionTrigger.php
@@ -87,27 +87,38 @@ private static function parseHashed(string $expression, string $context): string
             return $expression;
         }
 
+        $hashEngine = self::hashEngine($context);
+
         foreach ($parts as $position => $part) {
             if (preg_match('#^\#(\((\d+)-(\d+)\))?$#', $part, $matches)) {
-                $parts[$position] = self::hashField(
+                $parts[$position] = $hashEngine(
                     (int) ($matches[2] ?? self::HASH_RANGES[$position][0]),
                     (int) ($matches[3] ?? self::HASH_RANGES[$position][1]),
-                    $context,
                 );
             }
         }
 
         return implode(' ', $parts);
     }
 
-    private static function hashField(int $start, int $end, string $context): int
+    /**
+     * @return callable(int,int):int
+     */
+    private static function hashEngine(string $context): callable
     {
         if (class_exists(Randomizer::class)) {
-            return (new Randomizer(new Xoshiro256StarStar(hash('sha256', $context, true))))->getInt($start, $end);
+            $randomizer = new Randomizer(new Xoshiro256StarStar(hash('sha256', $context, true)));
+
+            return static fn ($start, $end) => $randomizer->getInt($start, $end);
         }
 
-        $possibleValues = range($start, $end);
+        $counter = 0;
+
+        return static function ($start, $end) use ($context, &$counter) {
+            $possibleValues = range($start, $end);
+            ++$counter;
 
-        return $possibleValues[(int) fmod(hexdec(substr(md5($context), 0, 10)), \count($possibleValues))];
+            return $possibleValues[(int) fmod(hexdec(substr(md5($context.'-'.$counter), 0, 10)), \count($possibleValues))];
+        };
     }
 }