From 45d5867e7c8f46a770bac8860cdbf74992b7bf8a Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 24 Mar 2026 12:07:58 -0500 Subject: [PATCH 01/10] Demystify `HomeserverTestCase.pump()` Spawning from https://github.com/element-hq/synapse/pull/18416#discussion_r2967619735 --- tests/unittest.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/tests/unittest.py b/tests/unittest.py index 6022c750d03..4784ca7d432 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -697,7 +697,21 @@ async def run_bg_updates() -> None: def pump(self, by: float = 0.0) -> None: """ - Pump the reactor enough that Deferreds will fire. + Pump the reactor enough that scheduled Deferreds will fire. + + To demystify, this function, it simply advances time by the number of seconds + specified (defaults to `0`, for some reason we also multiply by 100, so + `pump(1)` is 100 seconds) and run whatever tasks are queued/pending and now + ready to run because enough time as passed. + + It doesn't have anything to do with making the reactor run or magic like that. + + So for example, if you have some Synapse code that does + `clock.call_later(Duration(seconds=2), callback)`, then calling + `self.pump(0.02)` will advance time by 2 seconds, which is enough for that + callback to be ready to run now. Same for `clock.sleep(...)` , + `clock.looping_call(...)`, and whatever other clock utilities for scheduling + tasks. """ self.reactor.pump([by] * 100) From d5b190182566673751ed694c7b4f454657b1bc8f Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 24 Mar 2026 12:24:41 -0500 Subject: [PATCH 02/10] More comments --- tests/unittest.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/unittest.py b/tests/unittest.py index 4784ca7d432..440a9f0a0ad 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -701,8 +701,8 @@ def pump(self, by: float = 0.0) -> None: To demystify, this function, it simply advances time by the number of seconds specified (defaults to `0`, for some reason we also multiply by 100, so - `pump(1)` is 100 seconds) and run whatever tasks are queued/pending and now - ready to run because enough time as passed. + `pump(1)` is 100 seconds in 1 second increments) and run whatever tasks are + queued/pending and now ready to run because enough time as passed. It doesn't have anything to do with making the reactor run or magic like that. @@ -712,6 +712,9 @@ def pump(self, by: float = 0.0) -> None: callback to be ready to run now. Same for `clock.sleep(...)` , `clock.looping_call(...)`, and whatever other clock utilities for scheduling tasks. + + XXX: If you're having to call this function, please call out in comments, which + scheduled thing you're aiming to trigger. """ self.reactor.pump([by] * 100) From ce303ec4e620f3dea11f739d0ccfcaec62717134 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 24 Mar 2026 12:30:58 -0500 Subject: [PATCH 03/10] Fix wording --- tests/unittest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unittest.py b/tests/unittest.py index 440a9f0a0ad..2198c132774 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -699,7 +699,7 @@ def pump(self, by: float = 0.0) -> None: """ Pump the reactor enough that scheduled Deferreds will fire. - To demystify, this function, it simply advances time by the number of seconds + To demystify, this function simply advances time by the number of seconds specified (defaults to `0`, for some reason we also multiply by 100, so `pump(1)` is 100 seconds in 1 second increments) and run whatever tasks are queued/pending and now ready to run because enough time as passed. From 972585426fbeb0205dd488dd9862c9a329fd0d5a Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 24 Mar 2026 12:31:34 -0500 Subject: [PATCH 04/10] Add changelog --- changelog.d/19602.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/19602.misc diff --git a/changelog.d/19602.misc b/changelog.d/19602.misc new file mode 100644 index 00000000000..222f26980db --- /dev/null +++ b/changelog.d/19602.misc @@ -0,0 +1 @@ +Update `HomeserverTestCase.pump()` docstring to demystify behavior (Twisted reactor/clock). From 838a808217782e19796c5d6d7a16dd05a3708ec0 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 24 Mar 2026 13:14:31 -0500 Subject: [PATCH 05/10] Deprecate `pump()` and refactor wording --- tests/unittest.py | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/tests/unittest.py b/tests/unittest.py index 2198c132774..5dc8a1418eb 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -697,25 +697,41 @@ async def run_bg_updates() -> None: def pump(self, by: float = 0.0) -> None: """ - Pump the reactor enough that scheduled Deferreds will fire. + XXX: Deprecated: This method is deprecated. Use `self.reactor.advance(...)` + directly instead. - To demystify, this function simply advances time by the number of seconds - specified (defaults to `0`, for some reason we also multiply by 100, so - `pump(1)` is 100 seconds in 1 second increments) and run whatever tasks are - queued/pending and now ready to run because enough time as passed. + Pump the reactor enough that scheduled Deferreds will fire. - It doesn't have anything to do with making the reactor run or magic like that. + To demystify this function, it simply advances time by the number of seconds + specified (defaults to `0`, we also multiply by 100, so `pump(1)` is 100 seconds + in 1 second increments) which allows any queued/pending tasks to run because + enough time has passed. So for example, if you have some Synapse code that does `clock.call_later(Duration(seconds=2), callback)`, then calling - `self.pump(0.02)` will advance time by 2 seconds, which is enough for that + `self.pump(by=0.02)` will advance time by 2 seconds, which is enough for that callback to be ready to run now. Same for `clock.sleep(...)` , `clock.looping_call(...)`, and whatever other clock utilities for scheduling - tasks. + tasks. Trying to use `pump(by=...)` with exact math to meet a specific deadline + feels pretty dirty though which is why we recommend using + `self.reactor.advance(...)` directly nowadays. + + We don't have any exact historical context for why `pump()` was introduced into + the codebase beyond the code itself. We assume that we multiply by 100 so that + when you create a Deferred that create more Deferreds, it tries to run the whole + chain to completion. XXX: If you're having to call this function, please call out in comments, which - scheduled thing you're aiming to trigger. + scheduled thing you're aiming to trigger. Please also check whether the + `pump(...)` is even necessary as it was often misused. + + Args: + by: The time increment in seconds to advance time by. We will advance time + 100x by this value. """ + # We multiply by 100, so `pump(1)` actually advances time by 100 seconds in 1 + # second increments. We assume this was done so that when you create a Deferred + # that create more Deferreds, it tries to run the whole chain to completion. self.reactor.pump([by] * 100) def get_success(self, d: Awaitable[TV], by: float = 0.0) -> TV: From 65efb52f8d770ff44e3fef23763dcc57a873b012 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 24 Mar 2026 13:17:44 -0500 Subject: [PATCH 06/10] Update changelog for deprecation --- changelog.d/{19602.misc => 19602.misc.1} | 0 changelog.d/19602.misc.2 | 1 + 2 files changed, 1 insertion(+) rename changelog.d/{19602.misc => 19602.misc.1} (100%) create mode 100644 changelog.d/19602.misc.2 diff --git a/changelog.d/19602.misc b/changelog.d/19602.misc.1 similarity index 100% rename from changelog.d/19602.misc rename to changelog.d/19602.misc.1 diff --git a/changelog.d/19602.misc.2 b/changelog.d/19602.misc.2 new file mode 100644 index 00000000000..4cadad653cf --- /dev/null +++ b/changelog.d/19602.misc.2 @@ -0,0 +1 @@ +Deprecate `HomeserverTestCase.pump()` in favor of more direct `HomeserverTestCase.reactor.advance(...)` usage. From 0a432d9a14dda1008435572b1717a157f39d8cc2 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 24 Mar 2026 13:57:58 -0500 Subject: [PATCH 07/10] Use "steps" to better illustrate discrete increments See https://github.com/element-hq/synapse/pull/19602#discussion_r2983564328 --- tests/unittest.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/unittest.py b/tests/unittest.py index 5dc8a1418eb..3b5c33ca0bc 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -704,8 +704,8 @@ def pump(self, by: float = 0.0) -> None: To demystify this function, it simply advances time by the number of seconds specified (defaults to `0`, we also multiply by 100, so `pump(1)` is 100 seconds - in 1 second increments) which allows any queued/pending tasks to run because - enough time has passed. + in 1 second steps/increments) which allows any queued/pending tasks to run + because enough time has passed. So for example, if you have some Synapse code that does `clock.call_later(Duration(seconds=2), callback)`, then calling @@ -727,11 +727,12 @@ def pump(self, by: float = 0.0) -> None: Args: by: The time increment in seconds to advance time by. We will advance time - 100x by this value. + in 100 steps, each step by this value. """ # We multiply by 100, so `pump(1)` actually advances time by 100 seconds in 1 - # second increments. We assume this was done so that when you create a Deferred - # that create more Deferreds, it tries to run the whole chain to completion. + # second steps/increments. We assume this was done so that when you create a + # Deferred that create more Deferreds, it tries to run the whole chain to + # completion. self.reactor.pump([by] * 100) def get_success(self, d: Awaitable[TV], by: float = 0.0) -> TV: From 9547b8d30d7bf771b41ccc09110df73cf75738ca Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 24 Mar 2026 14:07:11 -0500 Subject: [PATCH 08/10] Advance time and run callbacks See https://github.com/element-hq/synapse/pull/19602#discussion_r2983527788 --- tests/unittest.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unittest.py b/tests/unittest.py index 3b5c33ca0bc..f54e335641c 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -704,8 +704,8 @@ def pump(self, by: float = 0.0) -> None: To demystify this function, it simply advances time by the number of seconds specified (defaults to `0`, we also multiply by 100, so `pump(1)` is 100 seconds - in 1 second steps/increments) which allows any queued/pending tasks to run - because enough time has passed. + in 1 second steps/increments) whilst calling any pending callbacks, allowing any + queued/pending tasks to run because enough time has passed. So for example, if you have some Synapse code that does `clock.call_later(Duration(seconds=2), callback)`, then calling From f79c3a46281a47ef0573966c3df3dbbb58408a8d Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 25 Mar 2026 12:08:58 -0500 Subject: [PATCH 09/10] Deferreds is a red herring See https://github.com/element-hq/synapse/pull/19602#discussion_r2983709268 --- tests/unittest.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/unittest.py b/tests/unittest.py index f54e335641c..66a88d3990e 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -700,7 +700,7 @@ def pump(self, by: float = 0.0) -> None: XXX: Deprecated: This method is deprecated. Use `self.reactor.advance(...)` directly instead. - Pump the reactor enough that scheduled Deferreds will fire. + Pump the reactor enough that clock scheduled callbacks will fire. To demystify this function, it simply advances time by the number of seconds specified (defaults to `0`, we also multiply by 100, so `pump(1)` is 100 seconds @@ -718,8 +718,8 @@ def pump(self, by: float = 0.0) -> None: We don't have any exact historical context for why `pump()` was introduced into the codebase beyond the code itself. We assume that we multiply by 100 so that - when you create a Deferred that create more Deferreds, it tries to run the whole - chain to completion. + when you use the clock to schedule something that schedules more things, it + tries to run the whole chain to completion. XXX: If you're having to call this function, please call out in comments, which scheduled thing you're aiming to trigger. Please also check whether the @@ -730,9 +730,9 @@ def pump(self, by: float = 0.0) -> None: in 100 steps, each step by this value. """ # We multiply by 100, so `pump(1)` actually advances time by 100 seconds in 1 - # second steps/increments. We assume this was done so that when you create a - # Deferred that create more Deferreds, it tries to run the whole chain to - # completion. + # second steps/increments. We assume this was done so that when you use the + # clock to schedule something that schedules more things, it tries to run the + # whole chain to completion. self.reactor.pump([by] * 100) def get_success(self, d: Awaitable[TV], by: float = 0.0) -> TV: From bf537ae75b6e3abe04fac11c63eac8978faf90b8 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 25 Mar 2026 12:19:03 -0500 Subject: [PATCH 10/10] Specify `clock.call_later` only --- tests/unittest.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/unittest.py b/tests/unittest.py index 66a88d3990e..03db9a42827 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -700,7 +700,7 @@ def pump(self, by: float = 0.0) -> None: XXX: Deprecated: This method is deprecated. Use `self.reactor.advance(...)` directly instead. - Pump the reactor enough that clock scheduled callbacks will fire. + Pump the reactor enough that `clock.call_later` scheduled callbacks will fire. To demystify this function, it simply advances time by the number of seconds specified (defaults to `0`, we also multiply by 100, so `pump(1)` is 100 seconds @@ -711,10 +711,11 @@ def pump(self, by: float = 0.0) -> None: `clock.call_later(Duration(seconds=2), callback)`, then calling `self.pump(by=0.02)` will advance time by 2 seconds, which is enough for that callback to be ready to run now. Same for `clock.sleep(...)` , - `clock.looping_call(...)`, and whatever other clock utilities for scheduling - tasks. Trying to use `pump(by=...)` with exact math to meet a specific deadline - feels pretty dirty though which is why we recommend using - `self.reactor.advance(...)` directly nowadays. + `clock.looping_call(...)`, and whatever other clock utilities that use + `clock.call_later` under the hood for scheduling tasks. Trying to use + `pump(by=...)` with exact math to meet a specific deadline feels pretty dirty + though which is why we recommend using `self.reactor.advance(...)` directly + nowadays. We don't have any exact historical context for why `pump()` was introduced into the codebase beyond the code itself. We assume that we multiply by 100 so that