Prefer make_awaitable over defer.succeed in tests

[squahtx]

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.

[richvdh]

generally lgtm, a couple of suggestions

[richvdh]

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

[squahtx]

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.

[richvdh]

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]

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]

Done

[squahtx]

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)

[squahtx]

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.

[richvdh]

well, that sounds good!

[richvdh]

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

[squahtx]

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]

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

[richvdh]

lgtm either way

[richvdh]

I'd do

        async def awaiter() -> R:
            return await res

        res = defer.ensureDeferred(awaiter())

(or, move awaiter to top-level)

but ymmv.

[squahtx]

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.