Safe async event cache

[Fizzadar]

Safer get event cache invalidation per discussion on #synapse-dev. The key here is to separate the async & local invalidation calls and ensure any async call is executed first.

Signed off by Nick @ Beeper (@Fizzadar).

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

[clokep]

For cross-referencing -- this is a follow-up to #13242, see the conversation at https://matrix.to/#/!vcyiEtMVHIhWXcJAfl:sw1v.org/$4eMMzxF4K1DfwLbQ3k4PAo1OTEmsLjGZm2d-DdaG0lQ?via=matrix.org&via=element.io&via=beeper.com

[richvdh]

can you add some comments here please? What is going on here, and why?

[Fizzadar]

Added a docstring to describe it - should this be hoisted to a class method now perhaps? b8ea213

[richvdh]

should this be hoisted to a class method now perhaps?

or, better, a top-level function. Though I suspect the changes regarding async_call_after mean this function isn't going to be useful any more.

[richvdh]

is there a good reason to limit this to coroutine functions, rather than other awaitables? I think inspect.isawaitable(cb) would be better?

[Fizzadar]

That's what I did first but isawaitable is only true on the return value of the function (isawaitable(cb())) rather than the function def itself:

In [1]: async def function():
   ...:     pass

In [2]: class Test:
   ...:     async def method():
   ...:         pass
 
In [4]: inspect.isawaitable(function)
Out[4]: False
In [5]: inspect.isawaitable(Test.method)
Out[5]: False
In [6]: inspect.iscoroutine(function)
Out[6]: False
In [7]: inspect.iscoroutine(Test.method)
Out[7]: False
In [8]: inspect.iscoroutinefunction(function)
Out[8]: True
In [9]: inspect.iscoroutinefunction(Test.method)
Out[9]: True

Perhaps this should be changed to also check awaitables?

[richvdh]

oh sorry, brainfart on my part. As you say: coroutine functions aren't themselves awaitable; they return an awaitable the first time you call them.

Unfortunately, they aren't the only thing that can return awaitables (eg: old-style functions that just return a Twisted Deferred object) and we need to do the same thing with other such functions. And you can't tell whether that is the case (other than via typing annotations) until you call the function in question and see what you get back - which of course doesn't work here.

So I think the only correct way to do this is to have separate txn.async_call_after which updates a separate list of async callbacks.

[richvdh]

IMHO this second paragraph belongs as a comment rather than a docstring. It's not really the place of a docstring to describe the reasons behind implementation decisions within a method.

[richvdh]

should this be hoisted to a class method now perhaps?

or, better, a top-level function. Though I suspect the changes regarding async_call_after mean this function isn't going to be useful any more.

[richvdh]

looks good, thanks!