Make autojump/autojump_threshold methods of abc.Clock

[fjarri]

Fixes #3369 by exposing autojump and autojump_threshold as methods of abc.Clock. runner.clock_autojump_threshold is removed, and the runner now gets it from clock.

TODO: adjust documentation (autoump_threshold description can be moved from MockClock to abc.Clock)

Note that MockClock still has some internal trickery related to guest mode, but it is only necessary if you want to modify autojump_threshold while the loop is running. I am not sure how to resolve that at the moment, but I'll think about it.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.99484%. Comparing base (c0fa9bc) to head (76524e5).
⚠️ Report is 1 commits behind head on main.

❌ Your project check has failed because the head coverage (99.99484%) is below the target coverage (100.00000%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@                 Coverage Diff                  @@
##                 main       #3371         +/-   ##
====================================================
- Coverage   100.00000%   99.99484%   -0.00517%     
====================================================
  Files             128         128                 
  Lines           19407       19398          -9     
  Branches         1318        1316          -2     
====================================================
- Hits            19407       19397         -10     
- Partials            0           1          +1     

Files with missing lines	Coverage Δ
src/trio/_abc.py	100.00000% <100.00000%> (ø)
src/trio/_core/_mock_clock.py	100.00000% <100.00000%> (ø)
src/trio/_core/_run.py	99.90494% <100.00000%> (-0.09507%)	⬇️
src/trio/_core/_tests/test_mock_clock.py	100.00000% <100.00000%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[fjarri]

Following some discussion in #3369, the first commit now represents the minimum change - the new methods in the ABC have default implementations, so the PR is not breaking.

But the whole handling of the clock in the event loops feels a little weird to me. So I tried to re-imagine the logic in the second commit.

The idea is, we have real time (system time), and virtual time (clock time). These should not be mixed. The runner's statistics work with virtual times. In unrolled_run(), the return time of runner.deadlines.next_deadline(), or the timeouts in runner.waiting_for_idle are all virtual times. But when we call events = yield timeout, this is real time - we will actually sleep for the requested time.

Thus the responsibility of the clock is to keep virtual time, and to convert it to real time when necessary. When we yield, some real time may pass (possibly less than requested), so we need to tell the clock that. Obviously, SystemClock will already know it via the side effect, so it can just ignore it, but MockClock needs to know. The abstract API of a clock is then:

• current_time() (naturally)
• deadline_to_sleep_time(timeout: float) -> float (probably to be renamed) - given the virtual timeout, returns the real time to sleep
• propagate(real_time_passed: float, virtual_timeout: float) - a new method, telling the clock that we slept for some real time derived from virtual_timeout, and the clock needs to be updated accordingly.

The second argument in propagate is really only needed to handle MockClock with rate = 0, whose logic I still don't completely understand. The docs say that "the clock only advances through manual calls to jump", and it's true technically, but an important detail is that the runner makes these calls too. The question is, if the autojump threshold is 2 seconds, and we sleep for 1 second, should the clock advance? With the original logic it doesn't, but I think it should.

[A5rocks]

Sorry, I've been out of commission for a while... Strictly looking at the code this is still a breaking change (deadline_to_sleep_time shouldn't have its argument renamed).

propagate feels like a good idea (at least as presented), actually. But I'm not sure about that second arg, my impression was that rate=0 means that no matter how much real time passes, no virtual time will pass. I'm not sure if I'm doing some magical thinking, but could we potentially avoid changing deadline_to_sleep_time to return timeouts and instead only need to add propagate? -snipped because I wasn't thinking things through-

This would be so simple if only we had no runner.waiting_for_idle to worry about!

[A5rocks]

OK I played around with things until I have something that passes our test suite, with a bit of cheating: I added a new statistic tracking how many tasks are waiting for an idle period (i.e. in wait_all_tasks_blocked).

Here's the very hacky diff if you want to see:

Details

diff --git a/src/trio/_abc.py b/src/trio/_abc.py

index abb6824381..2c3982d055 100644
--- a/src/trio/_abc.py
+++ b/src/trio/_abc.py
@@ -65,6 +65,9 @@

         """

+    def propagate(self, timeout: float) -> None:  # TODO: think about migration
+        pass
+

 class Instrument(ABC):  # noqa: B024  # conceptually is ABC
     """The interface for run loop instrumentation.
diff --git a/src/trio/_core/_mock_clock.py b/src/trio/_core/_mock_clock.py
index e437464b65..0bf738a61c 100644
--- a/src/trio/_core/_mock_clock.py
+++ b/src/trio/_core/_mock_clock.py
@@ -70,6 +70,7 @@
         self._real_base = 0.0
         self._virtual_base = 0.0
         self._rate = 0.0
+        self._jump_to: float | None = None

         # kept as an attribute so that our tests can monkeypatch it
         self._real_clock = time.perf_counter
@@ -118,9 +119,9 @@
                 runner.force_guest_tick_asap()
         except AttributeError:
             pass
-        else:
-            if runner.clock is self:
-                runner.clock_autojump_threshold = self._autojump_threshold
+        # else:
+        #     if runner.clock is self:
+        #         runner.clock_autojump_threshold = self._autojump_threshold

     # Invoked by the run loop when runner.clock_autojump_threshold is
     # exceeded.
@@ -145,6 +146,12 @@
         virtual_timeout = deadline - self.current_time()
         if virtual_timeout <= 0:
             return 0
+        elif (
+            self.autojump_threshold * self._rate <= virtual_timeout and
+            _core.current_statistics().idle_waiters == 0
+        ):
+            self._jump_to = deadline
+            return self.autojump_threshold
         elif self._rate > 0:
             return virtual_timeout / self._rate
         else:
@@ -163,3 +170,11 @@
         if seconds < 0:
             raise ValueError("time can't go backwards")
         self._virtual_base += seconds
+
+    def propagate(self, timeout: float) -> None:
+        # we base most parts off a system clock, so this is unnecessary
+        # except for:
+        if self._jump_to:
+            # we really should just rely on `self._jump_to`...
+            self._autojump()  # temp
+            self._jump_to = None
diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py
index 8a7ddc1dc3..25f262fa59 100644
--- a/src/trio/_core/_run.py
+++ b/src/trio/_core/_run.py
@@ -1709,6 +1709,8 @@
       backend. This always has an attribute ``backend`` which is a string
       naming which operating-system-specific I/O backend is in use; the
       other attributes vary between backends.
+    * ``idle_waiters`` (int): The number of tasks waiting in
+      `Runner.wait_all_tasks_blocked`.
     """

     tasks_living: int
@@ -1716,6 +1718,7 @@
     seconds_to_next_deadline: float
     io_statistics: IOStatistics
     run_sync_soon_queue_size: int
+    idle_waiters: int


 # This holds all the state that gets trampolined back and forth between
@@ -1817,7 +1820,7 @@
     asyncgens: AsyncGenerators = attrs.Factory(AsyncGenerators)

     # If everything goes idle for this long, we call clock._autojump()
-    clock_autojump_threshold: float = inf
+    # clock_autojump_threshold: float = inf

     # Guest mode stuff
     is_guest: bool = False
@@ -1869,6 +1872,7 @@
             seconds_to_next_deadline=seconds_to_next_deadline,
             io_statistics=self.io_manager.statistics(),
             run_sync_soon_queue_size=self.entry_queue.size(),
+            idle_waiters=len(self.waiting_for_idle),
         )

     @_public
@@ -2758,9 +2762,9 @@
             # We use 'elif' here because if there are tasks in
             # wait_all_tasks_blocked, then those tasks will wake up without
             # jumping the clock, so we don't need to autojump.
-            elif runner.clock_autojump_threshold < timeout:
-                timeout = runner.clock_autojump_threshold
-                idle_primed = IdlePrimedTypes.AUTOJUMP_CLOCK
+            # elif runner.clock_autojump_threshold < timeout:
+            #     timeout = runner.clock_autojump_threshold
+            #     idle_primed = IdlePrimedTypes.AUTOJUMP_CLOCK

             if "before_io_wait" in runner.instruments:
                 runner.instruments.call("before_io_wait", timeout)
@@ -2773,6 +2777,8 @@
             if "after_io_wait" in runner.instruments:
                 runner.instruments.call("after_io_wait", timeout)

+            runner.clock.propagate(timeout)
+
             # Process cancellations due to deadline expiry
             now = runner.clock.current_time()
             if runner.deadlines.expire(now):
@@ -2808,10 +2814,6 @@
                             runner.reschedule(task)
                         else:
                             break
-                else:
-                    assert idle_primed is IdlePrimedTypes.AUTOJUMP_CLOCK
-                    assert isinstance(runner.clock, _core.MockClock)
-                    runner.clock._autojump()

             # Process all runnable tasks, but only the ones that are already
             # runnable now. Anything that becomes runnable during this cycle
diff --git a/src/trio/_core/_tests/test_guest_mode.py b/src/trio/_core/_tests/test_guest_mode.py
index 743eddc846..649f71d6ab 100644
--- a/src/trio/_core/_tests/test_guest_mode.py
+++ b/src/trio/_core/_tests/test_guest_mode.py
@@ -655,7 +655,7 @@

     assert signal.getsignal(signal.SIGINT) is signal.default_int_handler

-
+@pytest.mark.skip("this breaks")
 def test_guest_mode_autojump_clock_threshold_changing() -> None:
     # This is super obscure and probably no-one will ever notice, but
     # technically mutating the MockClock.autojump_threshold from the host
diff --git a/src/trio/_core/_tests/test_mock_clock.py b/src/trio/_core/_tests/test_mock_clock.py
index 6131e7ba3f..26776f442a 100644
--- a/src/trio/_core/_tests/test_mock_clock.py
+++ b/src/trio/_core/_tests/test_mock_clock.py
@@ -177,7 +177,7 @@

     assert record == ["waiter done", "yawn"]

-
+@pytest.mark.skip("is no longer necessary")
 async def test_initialization_doesnt_mutate_runner() -> None:
     before = (
         GLOBAL_RUN_CONTEXT.runner.clock,

If it's fine by you (I assume you've already added a workaround), I'd like to propose this design in the issue and get others to weigh in?

[fjarri]

Strictly looking at the code this is still a breaking change (deadline_to_sleep_time shouldn't have its argument renamed).

The full PR yes, but but just the first commit is non-breaking.

I assume you've already added a workaround

Oh yes, it's not a blocker or anything. We can take our time.

I'd like to propose this design in the issue and get others to weigh in?

Sounds good to me.

I assume your diff is based off trunk? Seems like a simpler solution, and it does get rid of the isinstance(runner.clock, _core.MockClock) check which was my main problem. Need to think about the logic though, specifically on why idle_waiters is needed.

I noticed though that you commented out the test that modifies the autojump threshold from a host runner (which was an issue I didn't know how to resolve) - does that mean this functionality can be dropped?

[A5rocks]

I noticed though that you commented out the test that modifies the autojump threshold from a host runner (which was an issue I didn't know how to resolve) - does that mean this functionality can be dropped?

Yeah, in my opinion this is not an important property (we don't promise it in the docstrings I checked). But I guess I should also mention that in a proposal. (I guess this is also breaking :P)

specifically on why idle_waiters is needed.

My thought process was that wait_all_tasks_blocked should return before any autojump clocks, so we need to trigger them by returning a gigantic real timeout. But yes please do think through everything, I probably missed issues... And yeah, it's based off main.

An alternative to idle_waiters I think is what you did here w/r/t passing a timeout to a clock instead of a deadline... But this means that wait_all_tasks_blocked will be based on virtual rather than real timeouts (not important) and clocks don't get to govern the deadline -> timeout process... which may not be important, but may be? To be honest, I don't really get why clocks are an abstraction here and what purpose this could serve.

Hopefully I get the motivation to write up the proposal promptly!