Demystify and deprecate `HomeserverTestCase.pump()` (Twisted reactor/clock) by MadLittleMods · Pull Request #19602 · element-hq/synapse

MadLittleMods · 2026-03-24T17:09:48Z

Demystify and deprecate HomeserverTestCase.pump() (Twisted reactor/clock)

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
- Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
Code style is correct (run the linters)

MadLittleMods · 2026-03-24T17:25:06Z

tests/unittest.py

+        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


for some reason we also multiply by 100, so pump(1) is 100 seconds in 1 second increments

It looks like pump(...) was first introduced as self.reactor.pump([0.0] * 100) (matrix-org/synapse#3725) without explanation. I can only assume this was done so that when you create a deferred which creates more deferreds, it tries to run the whole chain to completion.

Then was updated to self.reactor.pump([by] * 100) (matrix-org/synapse#3740) without explanation. I assume some scheduled calls were in the mix and this was the patch.

Being enough to run the full chain has always been my understanding (or should I say assumption)

Added this assumed context to the docstring.

reivilibre

(side notes)

tests/unittest.py

reivilibre · 2026-03-24T17:31:55Z

tests/unittest.py

+
+        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


true, but relying on the multiplication factor being 100 and writing this to meet a specific deadline feels pretty dirty.
(Doesn't mean we don't do it)

I'm pretty sure there is also .advance somewhere that does a one-off advance with the given duration, that feels a little more sensible and maybe using pump everywhere is an antipattern

👍 on using self.reactor.advance(...) directly.

I can call out this function as deprecated and point out the alternative. Updated ✅

Now wondering if we ought to add a self.advance so it's as easy/obvious as self.pump. I think the advance name also makes it sound less magical, but could be imaginary

I could see as it as being useful to add our own docstring context to similarly demystify but we already self.reactor.advance(...) in many places and I'd rather not create another abstraction at this point.

reivilibre · 2026-03-24T17:33:47Z

tests/unittest.py

+        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


Being enough to run the full chain has always been my understanding (or should I say assumption)

reivilibre

honestly anything you write is going to be better than what was there, just you opening this PR is giving me time to reflect deeper on this, which is not a bad thing :)

reivilibre · 2026-03-24T18:26:24Z

tests/unittest.py

    def pump(self, by: float = 0.0) -> None:
        """
-        Pump the reactor enough that Deferreds will fire.
+        XXX: Deprecated: This method is deprecated. Use `self.reactor.advance(...)`


though even if we signpost advance more, maybe deprecating pump is a bit far. Making people write for _ in range(5): self.advance(0) in places may not necessarily make things more readable. Though in that case we could have a more explicit alternative advance_many where the 100 is replaced with an actual number.
Hard to say.

My suspicion is that most pump() usage is bogus. And in the cases where it does something, we really just need to advance time for a scheduled thing to happen.

I'm ok with moving forward with deprecating as it encourages bad behavior.

I'm coming around to this, yes let's deprecate it. Advancing 100 times internally 'just because' feels like bad practice to me.

I'd rather have a wrapper around advance

def advance(by = 0.0, require_calls = True)

or some such.

require_calls (true by default) would assert that there is actually something to progress. I am not sure whether to define that to mean 'there is something on the queue' or (stronger) 'there is something on the queue that actually gets executed by this advance call'.

But this is derailing this PR, so let's leave it for later (maybe I have a crack at this later today for the curiosity)

reivilibre · 2026-03-24T18:28:19Z

tests/unittest.py

+        Pump the reactor enough that scheduled Deferreds will fire.
+
+        To demystify this function, it simply advances time by the number of seconds


Suggested change

Pump the reactor enough that scheduled Deferreds will fire.

To demystify this function, it simply advances time by the number of seconds

Advances the test reactor's time by the number of seconds

Just thinking defining 'pump' as 'Pump' is not helping anyone, let's just skip to the chase and tell the reader what this does.

"Pump" is fine but it just wasn't clear "what happens"

tests/unittest.py

See #19602 (comment)

tests/unittest.py

reivilibre · 2026-03-25T11:11:53Z

tests/unittest.py

    def pump(self, by: float = 0.0) -> None:
        """
-        Pump the reactor enough that Deferreds will fire.
+        XXX: Deprecated: This method is deprecated. Use `self.reactor.advance(...)`


I'm coming around to this, yes let's deprecate it. Advancing 100 times internally 'just because' feels like bad practice to me.

I'd rather have a wrapper around advance

def advance(by = 0.0, require_calls = True)

or some such.

require_calls (true by default) would assert that there is actually something to progress. I am not sure whether to define that to mean 'there is something on the queue' or (stronger) 'there is something on the queue that actually gets executed by this advance call'.

But this is derailing this PR, so let's leave it for later (maybe I have a crack at this later today for the curiosity)

See #19602 (comment)

MadLittleMods · 2026-03-25T20:33:55Z

Thanks for the review @reivilibre 🦜

Comment is even more clear now 🧊

MadLittleMods added 2 commits March 24, 2026 12:07

Demystify HomeserverTestCase.pump()

45d5867

Spawning from #18416 (comment)

More comments

d5b1901

MadLittleMods commented Mar 24, 2026

View reviewed changes

MadLittleMods mentioned this pull request Mar 24, 2026

Limit outgoing to_device EDU size to 65536 #18416

Merged

3 tasks

MadLittleMods changed the title ~~Demystify HomeserverTestCase.pump()~~ Demystify HomeserverTestCase.pump() (Twisted reactor/clock) Mar 24, 2026

MadLittleMods added 2 commits March 24, 2026 12:30

Fix wording

ce303ec

Add changelog

9725854

MadLittleMods added the A-Testing label Mar 24, 2026

reivilibre reviewed Mar 24, 2026

View reviewed changes

Deprecate pump() and refactor wording

838a808

MadLittleMods changed the title ~~Demystify HomeserverTestCase.pump() (Twisted reactor/clock)~~ Demystify and deprecate HomeserverTestCase.pump() (Twisted reactor/clock) Mar 24, 2026

Update changelog for deprecation

65efb52

MadLittleMods requested a review from reivilibre March 24, 2026 18:18

MadLittleMods marked this pull request as ready for review March 24, 2026 18:19

MadLittleMods requested a review from a team as a code owner March 24, 2026 18:19

reivilibre approved these changes Mar 24, 2026

View reviewed changes

MadLittleMods added 2 commits March 24, 2026 13:57

Use "steps" to better illustrate discrete increments

0a432d9

See #19602 (comment)

Advance time and run callbacks

9547b8d

See #19602 (comment)

MadLittleMods requested a review from reivilibre March 24, 2026 19:12

Merge branch 'develop' into madlittlemods/demystify-pump

32f2ce2

reivilibre approved these changes Mar 25, 2026

View reviewed changes

MadLittleMods added 3 commits March 25, 2026 12:08

Deferreds is a red herring

f79c3a4

See #19602 (comment)

Merge branch 'develop' into madlittlemods/demystify-pump

8ec4973

Specify clock.call_later only

bf537ae

MadLittleMods merged commit f2b325f into develop Mar 25, 2026
40 checks passed

MadLittleMods deleted the madlittlemods/demystify-pump branch March 25, 2026 20:33

		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

		Pump the reactor enough that scheduled Deferreds will fire.

		To demystify this function, it simply advances time by the number of seconds

Conversation

MadLittleMods commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reivilibre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reivilibre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MadLittleMods commented Mar 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MadLittleMods commented Mar 24, 2026 •

edited

Loading

MadLittleMods Mar 24, 2026 •

edited

Loading

MadLittleMods Mar 24, 2026 •

edited

Loading