Prefer `make_awaitable` over `defer.succeed` in tests by squahtx · Pull Request #12505 · matrix-org/synapse

squahtx · 2022-04-19T19:27:02Z

When configuring the return values of mocks, prefer awaitables from
make_awaitable over defer.succeed. Deferreds are only awaitable
once, so it is inappropriate for a mock to return the same Deferred
multiple times.

Best reviewed commit by commit. The first commit modifies run_in_background to support the awaitables returned by make_awaitable. Without it, the tests in the third commit will fail.

Should unblock #12469, which failed CI because delay_cancellation "consumes" the result of Deferreds which can come from these mocks.

Signed-off-by: Sean Quah <seanq@element.io>

When configuring the return values of mocks, prefer awaitables from `make_awaitable` over `defer.succeed`. `Deferred`s are only awaitable once, so it is inappropriate for a mock to return the same `Deferred` multiple times. Signed-off-by: Sean Quah <seanq@element.io>

…st_transactions.py` Signed-off-by: Sean Quah <seanq@element.io>

Signed-off-by: Sean Quah <seanq@element.io>

tests/federation/test_federation_client.py

richvdh

generally lgtm, a couple of suggestions

richvdh · 2022-04-21T08:21:32Z

synapse/logging/context.py

    # At this point we should have a Deferred, if not then f was a synchronous
    # function, wrap it in a Deferred for consistency.


this comment (apart from being horrifying grammar) seems to be a bit outdated?

(I also wonder if we really need to support synchronous functions here these days. if not we can simplify all this with an assert isinstance(res, Awaitable))

I'll come up with some new words here.

Ditching support for synchronous functions is a good idea, but:

run_in_background is part of the module API so it's a little naughty to change it. However, I unknowingly did something similar to make_deferred_yieldable in Add missing type hints to synapse.logging.context #11556 by a while back and nobody's complained to my knowledge. So maybe that's fine?

run_in_background is called by preserve_fn, which also accepts synchronous functions. preserve_fn's used by @cached and @cachedList to wrap methods, some of which are still synchronous. I think restricting those decorators to async functions is going to turn out to be a rabbit hole.

ok, fair. In general, @cached and co seem to be overkill for a synchronous function and we should just use @lru_cache or something instead - but agreed this is a rabbithole we shouldn't get into right now.

richvdh · 2022-04-21T08:39:07Z

synapse/logging/context.py

-        assert not isinstance(res, Awaitable)
-
-        return defer.succeed(res)
+        if isinstance(res, Awaitable):


can we flip this condition to reduce nesting?

if not isinstance(res, Awaitable): # `f` returned a plain value. return defer.succeed(res) # now handle the completed awaitable case

squahtx · 2022-04-21T13:33:04Z

synapse/logging/context.py

+    # First we handle coroutines by wrapping them in a `Deferred`.
    if isinstance(res, typing.Coroutine):
        res = defer.ensureDeferred(res)

-    # At this point we should have a Deferred, if not then f was a synchronous
-    # function, wrap it in a Deferred for consistency.
+    # At this point, `res` may be a plain value, `Deferred`, or some other kind of
+    # non-coroutine awaitable.
    if not isinstance(res, defer.Deferred):
-        # `res` is not a `Deferred` and not a `Coroutine`.
-        # There are no other types of `Awaitable`s we expect to encounter in Synapse.
-        assert not isinstance(res, Awaitable)
-
-        return defer.succeed(res)
+        # Wrap plain values in a `Deferred`.
+        if not isinstance(res, Awaitable):
+            return defer.succeed(res)
+
+        # `res` is some kind of awaitable that is not a coroutine or `Deferred`.
+        # We assume that it is a completed awaitable, such as a `DoneAwaitable` or
+        # `Future` from `make_awaitable`, and await it manually.
+        iterator = res.__await__()  # `__await__` returns an iterator...
+        try:
+            next(iterator)
+            raise ValueError(f"Function {f} returned an unresolved awaitable: {res}")
+        except StopIteration as e:
+            # ...which raises a `StopIteration` once the awaitable is complete.
+            return defer.succeed(e.value)


I'm very tempted to replace all this with:

# `res` may be a coroutine, `Deferred`, some other kind of awaitable, or a plain # value. if isinstance(res, typing.Coroutine): # Wrap the coroutine in a `Deferred`. res = defer.ensureDeferred(res) elif isinstance(res, defer.Deferred): pass elif isinstance(res, Awaitable): # `res` is probably some kind of completed awaitable, such as a `DoneAwaitable` # or `Future` from `make_awaitable`. def awaiter(awaitable: Awaitable[R]): return await awaitable res = defer.ensureDeferred(awaiter(res)) else: # `res` is a plain value. Wrap it in a `Deferred`. return defer.succeed(res)

ie. get rid of the nested ifs and spawn a coroutine to deal with weird awaitables that we don't quite know how to handle.

well, that sounds good!

def awaiter(awaitable: Awaitable[R]): return await awaitable res = defer.ensureDeferred(awaiter(res))

if you're going to define a local function anyway, I'd go with:

async def awaiter(): return await res res = defer.ensureDeferred(awaiter())

I gave that a go and mypy wasn't convinced that res is an awaitable inside the function:

synapse/logging/context.py:816: error: Incompatible types in "await" (actual type "Union[R, Awaitable[R]]", expected type "Awaitable[Any]") [misc]

richvdh

lgtm. Your plan to write a single if ... elif... chain sounds good though.

squahtx · 2022-04-22T10:00:00Z

Did the mini-refactor to use a single if-elif chain. Let me know what you think!

richvdh

lgtm either way

richvdh · 2022-04-27T10:44:57Z

synapse/logging/context.py

+        async def awaiter(awaitable: Awaitable[R]) -> R:
+            return await awaitable

-    # At this point we should have a Deferred, if not then f was a synchronous
-    # function, wrap it in a Deferred for consistency.
-    if not isinstance(res, defer.Deferred):
-        # `res` is not a `Deferred` and not a `Coroutine`.
-        # There are no other types of `Awaitable`s we expect to encounter in Synapse.
-        assert not isinstance(res, Awaitable)
-
-        return defer.succeed(res)
+        res = defer.ensureDeferred(awaiter(res))


I'd do

async def awaiter() -> R: return await res res = defer.ensureDeferred(awaiter())

(or, move awaiter to top-level)

but ymmv.

mypy doesn't like that:

synapse/logging/context.py:816: error: Incompatible types in "await" (actual type "Union[R, Awaitable[R]]", expected type "Awaitable[Any]") [misc]

so I've lifted it out to the top level instead.

@SimonBrandner

Synapse 1.59.0rc1 (2022-05-10) ============================== This release makes several changes that server administrators should be aware of: - Device name lookup over federation is now disabled by default. ([\#12616](#12616)) - The `synapse.app.appservice` and `synapse.app.user_dir` worker application types are now deprecated. ([\#12452](#12452), [\#12654](#12654)) See [the upgrade notes](https://github.com/matrix-org/synapse/blob/develop/docs/upgrade.md#upgrading-to-v1590) for more details. Additionally, this release removes the non-standard `m.login.jwt` login type from Synapse. It can be replaced with `org.matrix.login.jwt` for identical behaviour. This is only used if `jwt_config.enabled` is set to `true` in the configuration. ([\#12597](#12597)) Features -------- - Support [MSC3266](matrix-org/matrix-spec-proposals#3266) room summaries over federation. ([\#11507](#11507)) - Implement [changes](matrix-org/matrix-spec-proposals@4a77139) to [MSC2285 (hidden read receipts)](matrix-org/matrix-spec-proposals#2285). Contributed by @SimonBrandner. ([\#12168](#12168), [\#12635](#12635), [\#12636](#12636), [\#12670](#12670)) - Extend the [module API](https://github.com/matrix-org/synapse/blob/release-v1.59/synapse/module_api/__init__.py) to allow modules to change actions for existing push rules of local users. ([\#12406](#12406)) - Add the `notify_appservices_from_worker` configuration option (superseding `notify_appservices`) to allow a generic worker to be designated as the worker to send traffic to Application Services. ([\#12452](#12452)) - Add the `update_user_directory_from_worker` configuration option (superseding `update_user_directory`) to allow a generic worker to be designated as the worker to update the user directory. ([\#12654](#12654)) - Add new `enable_registration_token_3pid_bypass` configuration option to allow registrations via token as an alternative to verifying a 3pid. ([\#12526](#12526)) - Implement [MSC3786](matrix-org/matrix-spec-proposals#3786): Add a default push rule to ignore `m.room.server_acl` events. ([\#12601](#12601)) - Add new `mau_appservice_trial_days` configuration option to specify a different trial period for users registered via an appservice. ([\#12619](#12619)) Bugfixes -------- - Fix a bug introduced in Synapse 1.48.0 where the latest thread reply provided failed to include the proper bundled aggregations. ([\#12273](#12273)) - Fix a bug introduced in Synapse 1.22.0 where attempting to send a large amount of read receipts to an application service all at once would result in duplicate content and abnormally high memory usage. Contributed by Brad & Nick @ Beeper. ([\#12544](#12544)) - Fix a bug introduced in Synapse 1.57.0 which could cause `Failed to calculate hosts in room` errors to be logged for outbound federation. ([\#12570](#12570)) - Fix a long-standing bug where status codes would almost always get logged as `200!`, irrespective of the actual status code, when clients disconnect before a request has finished processing. ([\#12580](#12580)) - Fix race when persisting an event and deleting a room that could lead to outbound federation breaking. ([\#12594](#12594)) - Fix a bug introduced in Synapse 1.53.0 where bundled aggregations for annotations/edits were incorrectly calculated. ([\#12633](#12633)) - Fix a long-standing bug where rooms containing power levels with string values could not be upgraded. ([\#12657](#12657)) - Prevent memory leak from reoccurring when presence is disabled. ([\#12656](#12656)) Updates to the Docker image --------------------------- - Explicitly opt-in to using [BuildKit-specific features](https://github.com/moby/buildkit/blob/master/frontend/dockerfile/docs/syntax.md) in the Dockerfile. This fixes issues with building images in some GitLab CI environments. ([\#12541](#12541)) - Update the "Build docker images" GitHub Actions workflow to use `docker/metadata-action` to generate docker image tags, instead of a custom shell script. Contributed by @henryclw. ([\#12573](#12573)) Improved Documentation ---------------------- - Update SQL statements and replace use of old table `user_stats_historical` in docs for Synapse Admins. ([\#12536](#12536)) - Add missing linebreak to `pipx` install instructions. ([\#12579](#12579)) - Add information about the TCP replication module to docs. ([\#12621](#12621)) - Fixes to the formatting of `README.rst`. ([\#12627](#12627)) - Fix docs on how to run specific Complement tests using the `complement.sh` test runner. ([\#12664](#12664)) Deprecations and Removals ------------------------- - Remove unstable identifiers from [MSC3069](matrix-org/matrix-spec-proposals#3069). ([\#12596](#12596)) - Remove the unspecified `m.login.jwt` login type and the unstable `uk.half-shot.msc2778.login.application_service` from [MSC2778](matrix-org/matrix-spec-proposals#2778). ([\#12597](#12597)) - Synapse now requires at least Python 3.7.1 (up from 3.7.0), for compatibility with the latest Twisted trunk. ([\#12613](#12613)) Internal Changes ---------------- - Use supervisord to supervise Postgres and Caddy in the Complement image to reduce restart time. ([\#12480](#12480)) - Immediately retry any requests that have backed off when a server comes back online. ([\#12500](#12500)) - Use `make_awaitable` instead of `defer.succeed` for return values of mocks in tests. ([\#12505](#12505)) - Consistently check if an object is a `frozendict`. ([\#12564](#12564)) - Protect module callbacks with read semantics against cancellation. ([\#12568](#12568)) - Improve comments and error messages around access tokens. ([\#12577](#12577)) - Improve docstrings for the receipts store. ([\#12581](#12581)) - Use constants for read-receipts in tests. ([\#12582](#12582)) - Log status code of cancelled requests as 499 and avoid logging stack traces for them. ([\#12587](#12587), [\#12663](#12663)) - Remove special-case for `twisted` logger from default log config. ([\#12589](#12589)) - Use `getClientAddress` instead of the deprecated `getClientIP`. ([\#12599](#12599)) - Add link to documentation in Grafana Dashboard. ([\#12602](#12602)) - Reduce log spam when running multiple event persisters. ([\#12610](#12610)) - Add extra debug logging to federation sender. ([\#12614](#12614)) - Prevent remote homeservers from requesting local user device names by default. ([\#12616](#12616)) - Add a consistency check on events which we read from the database. ([\#12620](#12620)) - Remove use of the `constantly` library and switch to enums for `EventRedactBehaviour`. Contributed by @andrewdoh. ([\#12624](#12624)) - Remove unused code related to receipts. ([\#12632](#12632)) - Minor improvements to the scripts for running Synapse in worker mode under Complement. ([\#12637](#12637)) - Move `pympler` back in to the `all` extras. ([\#12652](#12652)) - Fix spelling of `M_UNRECOGNIZED` in comments. ([\#12665](#12665)) - Release script: confirm the commit to be tagged before tagging. ([\#12556](#12556)) - Fix a typo in the announcement text generated by the Synapse release development script. ([\#12612](#12612)) - Fix scripts-dev to pass typechecking. ([\#12356](#12356)) - Add some type hints to datastore. ([\#12485](#12485)) - Remove unused `# type: ignore`s. ([\#12531](#12531)) - Allow unused `# type: ignore` comments in bleeding edge CI jobs. ([\#12576](#12576)) - Remove redundant lines of config from `mypy.ini`. ([\#12608](#12608)) - Update to mypy 0.950. ([\#12650](#12650)) - Use `Concatenate` to better annotate `_do_execute`. ([\#12666](#12666)) - Use `ParamSpec` to refine type hints. ([\#12667](#12667)) - Fix mypy against latest pillow stubs. ([\#12671](#12671))

Sean Quah added 4 commits April 19, 2022 20:20

Support DoneAwaitable and make_awaitable in run_in_background

2ff6425

Signed-off-by: Sean Quah <seanq@element.io>

Prefer make_awaitable over defer.succeed in `tests/rest/client/te…

301bfe3

…st_transactions.py` Signed-off-by: Sean Quah <seanq@element.io>

Add newsfile

9718ba7

Signed-off-by: Sean Quah <seanq@element.io>

squahtx requested a review from a team as a code owner April 19, 2022 19:27

clokep reviewed Apr 19, 2022

View reviewed changes

tests/federation/test_federation_client.py Show resolved Hide resolved

Appease mypy

180ab40

squahtx requested a review from a team April 20, 2022 17:29

richvdh reviewed Apr 21, 2022

View reviewed changes

Sean Quah added 2 commits April 21, 2022 14:27

Reword comments and reduce indent

629ce1b

run linter

ba28da5

squahtx commented Apr 21, 2022

View reviewed changes

squahtx requested a review from richvdh April 21, 2022 13:34

richvdh approved these changes Apr 21, 2022

View reviewed changes

refactor run_in_background

daa0e9a

squahtx requested a review from richvdh April 22, 2022 09:59

richvdh approved these changes Apr 27, 2022

View reviewed changes

Move awaiter function to top-level

8ffb491

squahtx merged commit 78b99de into develop Apr 27, 2022

squahtx deleted the squah/use_make_awaitable_in_tests_more branch April 27, 2022 13:58

This was referenced Apr 27, 2022

Add missing type hints to synapse.logging.context #11556

Merged

we shouldn't process requests where the requester has timed out/gone away #3528

Closed

		# At this point we should have a Deferred, if not then f was a synchronous
		# function, wrap it in a Deferred for consistency.

Uh oh!

Conversation

squahtx commented Apr 19, 2022

Uh oh!

Uh oh!

richvdh 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!

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!

Choose a reason for hiding this comment

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

squahtx commented Apr 22, 2022

Uh oh!

richvdh 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants