Merge pull request #4453 from tybug/free-threading-random-deprecation

Make `deprecate_random_in_strategy` thread safe

Author: tybug

hypothesis-python/RELEASE.rst

@@ -0,0 +1,3 @@
+RELEASE_TYPE: patch
+
+Makes the deprecation warning for using the global random instance thread-safe, as part of our work towards thread safety (:issue:`4451`).

hypothesis-python/src/hypothesis/control.py

@@ -112,10 +112,40 @@ def __call__(self, x):
 
 @contextmanager
 def deprecate_random_in_strategy(fmt, *args):
-    _global_rand_state = random.getstate()
+    from hypothesis.internal import entropy
+
+    state_before = random.getstate()
     yield (checker := _Checker())
-    if _global_rand_state != random.getstate() and not checker.saw_global_random:
-        # raise InvalidDefinition
+    state_after = random.getstate()
+    if (
+        # there is a threading race condition here with deterministic_PRNG. Say
+        # we have two threads 1 and 2. We start in global random state A, and
+        # deterministic_PRNG sets to global random state B (which is constant across
+        # threads since we seed to 0 unconditionally). Then we might have state
+        # transitions:
+        #
+        #  [1]        [2]
+        # A -> B                           deterministic_PRNG().__enter__
+        #            B ->B                 deterministic_PRNG().__enter__
+        #            state_before = B      deprecate_random_in_strategy.__enter__
+        # B -> A                           deterministic_PRNG().__exit__
+        #            state_after  = A      deprecate_random_in_strategy.__exit__
+        #
+        # where state_before != state_after because a different thread has reset
+        # the global random state.
+        #
+        # To fix this, we track the latest set global random state in
+        # deterministic_PRNG, and will not note a deprecation (or error, in the
+        # future) if the state afterwards is the same as the state that
+        # deterministic_PRNG set to.
+        state_after
+        not in [
+            state_before,
+            entropy._most_recent_random_state_enter,
+            entropy._most_recent_random_state_exit,
+        ]
+        and not checker.saw_global_random
+    ):
         note_deprecation(
             "Do not use the `random` module inside strategies; instead "
             "consider  `st.randoms()`, `st.sampled_from()`, etc.  " + fmt.format(*args),

hypothesis-python/src/hypothesis/internal/conjecture/junkdrawer.py

@@ -292,11 +292,12 @@ def stack_depth_of_caller() -> int:
 # recursion limit on exit to be any of the following:
 #
 # * the recursion limit on enter.
-# * the recursion limit as set by any other enter of ensure_free_stackframes.
+# * the recursion limit as set by the enter of the most recent
+#   ensure_free_stackframes.
 # * the recursion limit as set by the exit of the most recent
 #   ensure_free_stackframes.
-global_maxdepth_enters: list[int] = []
-most_recent_maxdepth_exit: Optional[int] = None
+_most_recent_maxdepth_enter: Optional[int] = None
+_most_recent_maxdepth_exit: Optional[int] = None
 
 
 class ensure_free_stackframes:
@@ -305,6 +306,7 @@ class ensure_free_stackframes:
     """
 
     def __enter__(self) -> None:
+        global _most_recent_maxdepth_enter
         cur_depth = stack_depth_of_caller()
         self.old_maxdepth = sys.getrecursionlimit()
         # The default CPython recursionlimit is 1000, but pytest seems to bump
@@ -321,23 +323,22 @@ def __enter__(self) -> None:
             "avoid extending the stack limit in an infinite loop..."
             % (self.new_maxdepth - self.old_maxdepth, self.old_maxdepth)
         )
-        global_maxdepth_enters.append(self.new_maxdepth)
+        _most_recent_maxdepth_enter = self.new_maxdepth
         sys.setrecursionlimit(self.new_maxdepth)
 
     def __exit__(self, *args, **kwargs):
-        global most_recent_maxdepth_exit
+        global _most_recent_maxdepth_exit
 
         # in single-threaded uses, we expect sys.getrecursionlimit == self.maxdepth.
         # The other checks are to avoid spurious warnings in multi-threaded
         # environments. Adding them slightly weakens this check, but acceptably so.
         if sys.getrecursionlimit() in [
             self.new_maxdepth,
-            *global_maxdepth_enters,
-            most_recent_maxdepth_exit,
+            _most_recent_maxdepth_enter,
+            _most_recent_maxdepth_exit,
         ]:
-            most_recent_maxdepth_exit = self.old_maxdepth
+            _most_recent_maxdepth_exit = self.old_maxdepth
             sys.setrecursionlimit(self.old_maxdepth)
-            global_maxdepth_enters.remove(self.new_maxdepth)
         else:  # pragma: no cover
             warnings.warn(
                 "The recursion limit will not be reset, since it was changed "

hypothesis-python/src/hypothesis/internal/entropy.py

@@ -35,12 +35,13 @@ def setstate(self, *args: Any, **kwargs: Any) -> Any: ...
 else:  # pragma: no cover
     RandomLike = random.Random
 
+_RKEY = count()
+_global_random_rkey = next(_RKEY)
 # This is effectively a WeakSet, which allows us to associate the saved states
 # with their respective Random instances even as new ones are registered and old
 # ones go out of scope and get garbage collected.  Keys are ascending integers.
-_RKEY = count()
 RANDOMS_TO_MANAGE: WeakValueDictionary[int, RandomLike] = WeakValueDictionary(
-    {next(_RKEY): random}
+    {_global_random_rkey: random}
 )
 
 
@@ -148,6 +149,17 @@ def my_WORKING_hook():
     RANDOMS_TO_MANAGE[next(_RKEY)] = r
 
 
+# the most recent state of the global random instance, as set by hypothesis.
+# This might not be the current state of the global random instance if its
+# state changed since hypothesis seeded it. If nobody other than hypothesis
+# is touching the global random instance, then this will be the state of the global
+# random instance.
+#
+# This is used to address a threading race condition in deprecate_random_in_strategy.
+_most_recent_random_state_enter: Optional[Any] = None
+_most_recent_random_state_exit: Optional[Any] = None
+
+
 def get_seeder_and_restorer(
     seed: Hashable = 0,
 ) -> tuple[Callable[[], None], Callable[[], None]]:
@@ -171,16 +183,34 @@ def get_seeder_and_restorer(
             NP_RANDOM = RANDOMS_TO_MANAGE[next(_RKEY)] = NumpyRandomWrapper()
 
     def seed_all() -> None:
+        global _most_recent_random_state_enter
         assert not states
         for k, r in RANDOMS_TO_MANAGE.items():
             states[k] = r.getstate()
             r.seed(seed)
+            if k == _global_random_rkey:
+                # setting a seed is equivalent to setting setstate, so we need
+                # to track it globally for race conditions.
+                #
+                # I think there's still a race here if a thread switch occurs
+                # after r.seed but before we set _most_recent_random_state_enter.
+                # We'd need to seed a dummy Random instance to figure out the
+                # seed -> state mapping, then set _most_recent_random_state_enter,
+                # then call setstate (or equivalently .seed) on the real random.
+                _most_recent_random_state_enter = r.getstate()
 
     def restore_all() -> None:
+        global _most_recent_random_state_exit
+
         for k, state in states.items():
             r = RANDOMS_TO_MANAGE.get(k)
-            if r is not None:  # i.e., hasn't been garbage-collected
-                r.setstate(state)
+            if r is None:  # i.e., has been garbage-collected
+                continue
+
+            if k == _global_random_rkey:
+                _most_recent_random_state_exit = state
+            r.setstate(state)
+
         states.clear()
 
     return seed_all, restore_all