Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Prefer make_awaitable over defer.succeed in tests #12505

Merged
merged 9 commits into from
Apr 27, 2022
Merged

Prefer make_awaitable over defer.succeed in tests #12505

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2ff6425
Support `DoneAwaitable` and `make_awaitable` in `run_in_background`
Apr 19, 2022
d56d118
Prefer `make_awaitable` over `defer.succeed` in tests
Apr 19, 2022
301bfe3
Prefer `make_awaitable` over `defer.succeed` in `tests/rest/client/te…
Apr 19, 2022
9718ba7
Add newsfile
Apr 19, 2022
180ab40
Appease mypy
Apr 19, 2022
629ce1b
Reword comments and reduce indent
Apr 21, 2022
ba28da5
run linter
Apr 21, 2022
daa0e9a
refactor run_in_background
Apr 22, 2022
8ffb491
Move `awaiter` function to top-level
Apr 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions synapse/logging/context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -808,10 +808,21 @@ def run_in_background( # type: ignore[misc]
# At this point we should have a Deferred, if not then f was a synchronous
# function, wrap it in a Deferred for consistency.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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)
if isinstance(res, Awaitable):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

# `f` returned 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.
res = e.value

# res is now a plain value here.
return defer.succeed(res)

if res.called and not res.paused:
Expand Down